[WEB-4921] refactor: replaced tabs implementation with propel tabs #7808
Conversation
…tionality and type safety
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces many Headless UI Tab usages with the @plane/propel Tabs API (Tabs, TabsList, TabsTrigger, TabsContent), converts index-based tab logic to value/onValueChange with typed tab keys, restructures tab content rendering, and overhauls the image picker with Unsplash search, grid, and Dropzone upload flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Tabs (value-controlled)
participant Router as NextRouter
participant Content as TabsContent
User->>UI: Click TabsTrigger (tabKey)
UI->>UI: onValueChange(tabKey)
UI->>Router: handleTabChange(tabKey) -> router.push(updated route/query)
Router-->>Content: Render TabsContent matching tabKey
sequenceDiagram
autonumber
actor User
participant Popover as ImagePickerPopover
participant Tabs as Tabs (Unsplash/Images/Upload)
participant Unsplash as SWR(unsplash)
participant Uploader as Upload API
User->>Tabs: Select "Unsplash"
Tabs->>Unsplash: fetch(search)
Unsplash-->>Tabs: Data / Loading
User->>Popover: Click image -> onChange(url), close
User->>Tabs: Select "Upload"
User->>Popover: Drop/select file
Popover->>Uploader: upload file
Uploader-->>Popover: URL
Popover->>User: onChange(url), reset, close
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Pull Request Overview
Refactored tabs implementation by replacing the existing tabs solution (primarily from @headlessui/react) with @plane/propel/tabs across multiple components for improved consistency and maintainability.
- Migrated all tabs from @headlessui/react Tab components to @plane/propel/tabs components
- Updated event handlers to use value-based callbacks instead of index-based
- Standardized tab styling and removed custom tab indicator logic
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/core/components/pages/version/editor.tsx | Reordered import statements |
| apps/web/core/components/pages/navigation-pane/tabs-list.tsx | Replaced headless UI tabs with propel tabs, simplified structure |
| apps/web/core/components/pages/navigation-pane/tab-panels/root.tsx | Updated tab panels to use TabsContent wrapper |
| apps/web/core/components/pages/navigation-pane/tab-panels/assets.tsx | Restructured conditional rendering logic and layout |
| apps/web/core/components/pages/navigation-pane/root.tsx | Updated root component to use propel tabs with value-based handlers |
| apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx | Migrated module progress stats from headless UI to propel tabs |
| apps/web/core/components/modules/analytics-sidebar/issue-progress.tsx | Removed obsolete roundedTab and noBackground props |
| apps/web/core/components/license/modal/card/base-paid-plan-card.tsx | Updated license plan cards to use propel tabs |
| apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx | Migrated cycle progress stats to propel tabs |
| apps/web/core/components/cycles/analytics-sidebar/issue-progress.tsx | Removed obsolete props from cycle analytics |
| apps/web/core/components/cycles/active-cycle/cycle-stats.tsx | Updated active cycle stats tabs implementation |
| apps/web/core/components/core/image-picker-popover.tsx | Major refactor of image picker with improved UI and propel tabs |
| apps/web/core/components/analytics/work-items/modal/content.tsx | Removed unnecessary Tab.Group wrapper |
| apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsx | Updated analytics page to use propel tabs with proper routing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
apps/web/core/components/pages/navigation-pane/tab-panels/root.tsx
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (19)
apps/web/core/components/analytics/work-items/modal/content.tsx (1)
71-76: Confirm intent: Tabs wrapper removed instead of migrating to Propel Tabs.If the modal no longer needs tab semantics/ARIA, this is fine. If it still represents multiple “sections” that previously were tabbed, migrate to
@plane/propel/tabsfor accessibility and consistency with the PR’s goal.apps/web/core/components/license/modal/card/base-paid-plan-card.tsx (3)
44-52: Ensure selected plan always matches available prices.If a plan is missing "month" (or data loads later),
value={selectedPlan}can point to a non-existent tab, rendering no content. Guard and fallback to the first availableprice.recurring.Apply this diff (adds useEffect and import):
-import { FC, useState } from "react"; +import { FC, useEffect, useState } from "react"; @@ const [selectedPlan, setSelectedPlan] = useState<TBillingFrequency>("month"); @@ + // keep selectedPlan in sync with available prices + useEffect(() => { + const available = new Set(prices.map((p) => p.recurring)); + if (!available.has(selectedPlan)) { + setSelectedPlan((prices[0]?.recurring as TBillingFrequency) ?? "month"); + } + }, [prices, selectedPlan]);
46-52: Avoid potential duplicate React keys and tab values.If
pricesever contains multiple entries with the samerecurring(e.g., currency variants), keys/values will collide. Use composite keys; keepvalueasrecurringif UX is strictly month/year.Apply this diff:
- {prices.map((price: TSubscriptionPrice) => ( - <TabsTrigger key={price.recurring} value={price.recurring} className="text-sm"> + {prices.map((price: TSubscriptionPrice, idx) => ( + <TabsTrigger key={`${price.recurring}-${idx}`} value={price.recurring} className="text-sm"> {renderPriceContent(price)} </TabsTrigger> ))} @@ - {prices.map((price: TSubscriptionPrice) => ( - <TabsContent key={price.recurring} value={price.recurring} className="px-2 pb-2"> + {prices.map((price: TSubscriptionPrice, idx) => ( + <TabsContent key={`${price.recurring}-${idx}`} value={price.recurring} className="px-2 pb-2"> <div className="text-center"> <div className="text-xl font-medium">Plane {planeName}</div> {renderActionButton(price)} </div> </TabsContent> ))}Also applies to: 53-61
64-82: Feature list item keys may collide.
key={feature}risks duplicates. Use a composite key.Apply this diff:
- {features.map((feature) => ( + {features.map((feature, idx) => ( <li - key={feature} + key={`${feature}-${idx}`}apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx (2)
202-221: Prefer stable keys over list index.Use the state group name if available to reduce unnecessary remounts.
Apply this diff:
- <SingleProgressStats - key={index} + <SingleProgressStats + key={group.state ?? String(index)}
309-348: Optional: lazy-mount heavy tab content.If
Tabssupports lazy/unmount behavior, enable it to avoid rendering three lists at once.apps/web/core/components/cycles/active-cycle/cycle-stats.tsx (3)
88-99: Sanitize persisted tab before passing to Tabs.If an old/invalid value exists in localStorage,
tabwon’t be falsy and the UI will render no active content. Sanitize to a known value.Apply this diff:
// plane hooks const { t } = useTranslation(); @@ - return cycleId ? ( + const allowedTabs: ReadonlyArray<TActiveCycleStatsTab> = ["Priority-Issues", "Assignees", "Labels"]; + const currentTab: TActiveCycleStatsTab = + allowedTabs.includes((tab as TActiveCycleStatsTab)) ? (tab as TActiveCycleStatsTab) : "Assignees"; + + return cycleId ? ( <div className="flex flex-col gap-4 p-4 min-h-[17rem] overflow-hidden bg-custom-background-100 col-span-1 lg:col-span-2 xl:col-span-1 border border-custom-border-200 rounded-lg"> - <Tabs value={tab || "Assignees"} onValueChange={handleTabChange} className="flex flex-col w-full h-full"> + <Tabs value={currentTab} onValueChange={handleTabChange} className="flex flex-col w-full h-full">
101-191: Localize hardcoded strings inside tooltips.“Title” and “Target Date” should use i18n keys for consistency.
252-289: Localize “No labels” fallback.Use a translation key (e.g.,
t("no_labels_yet")) for the label name fallback.Apply this diff:
- <span className="text-xs text-ellipsis truncate">{label.label_name ?? "No labels"}</span> + <span className="text-xs text-ellipsis truncate"> + {label.label_name ?? t("no_labels_yet")} + </span>apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx (2)
308-316: Localize user-facing strings ("Assignees", "Labels", "States", "No labels yet").These should use i18n for consistency with the rest of the module.
Apply:
- <TabsTrigger value="stat-assignees" size={size === "xs" ? "sm" : "md"}> - Assignees + <TabsTrigger value="stat-assignees" size={size === "xs" ? "sm" : "md"}> + {t("assignees")} </TabsTrigger> - <TabsTrigger value="stat-labels" size={size === "xs" ? "sm" : "md"}> - Labels + <TabsTrigger value="stat-labels" size={size === "xs" ? "sm" : "md"}> + {t("labels")} </TabsTrigger> - <TabsTrigger value="stat-states" size={size === "xs" ? "sm" : "md"}> - States + <TabsTrigger value="stat-states" size={size === "xs" ? "sm" : "md"}> + {t("states")} </TabsTrigger>- <h6 className="text-base text-custom-text-300">No labels yet</h6> + <h6 className="text-base text-custom-text-300">{t("no_labels_yet")}</h6>Also applies to: 184-184
154-155: Tighten label selection check.
label.idis guaranteed in this branch; the fallback to template string is dead code.- selected: filters?.filters?.labels?.includes(label.id ?? `no-label-${index}`), + selected: !!label.id && filters?.filters?.labels?.includes(label.id),apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsx (1)
63-67: Validate tabId against allowed keys before using it as the controlled value.Casting
tabIdblindly can lead to a blank view if the URL has an unknown tab.- const defaultTab = (tabId as TAnalyticsTabsBase) || ANALYTICS_TABS[0].key; + const validKeys = new Set(ANALYTICS_TABS.map((t) => t.key)); + const defaultTab = validKeys.has(tabId as TAnalyticsTabsBase) + ? (tabId as TAnalyticsTabsBase) + : (ANALYTICS_TABS[0].key as TAnalyticsTabsBase);Also applies to: 76-94
apps/web/core/components/core/image-picker-popover.tsx (3)
185-198: Default tab may be unavailable; derive a robust initial tab.If Unsplash errors and its trigger is hidden,
defaultValue="unsplash"leaves Tabs with no active tab.- <Tabs defaultValue="unsplash"> + {(() => { + const al 5100 lowed = tabOptions.filter((t) => { + if (t.key === "unsplash" && !unsplashImages && unsplashError) return false; + if (t.key === "images" && projectCoverImages && projectCoverImages.length === 0) return false; + return true; + }); + const initial = allowed[0]?.key ?? "upload"; + return ( + <Tabs defaultValue={initial}> + ); + })()}And close the IIFE before
</Tabs>.
10-10: Remove unused Headless UI Tab import.Only
Popoveris used now.-import { Tab, Popover } from "@headlessui/react"; +import { Popover } from "@headlessui/react";
349-356: Avoid object URL leaks when previewing uploads.Revoke the URL when
imagechanges or component unmounts.Add:
useEffect(() => { if (!image) return; const url = URL.createObjectURL(image); return () => URL.revokeObjectURL(url); }, [image]);And reuse
urlfor the preview.apps/web/core/components/pages/navigation-pane/root.tsx (4)
48-52: Guard against invalid query param values to prevent blank tabs.A rogue or stale ?paneTab value will render no active tab. Add a runtime check against allowed keys before assigning activeTab.
Apply this diff:
- const navigationPaneQueryParam = searchParams.get( - PAGE_NAVIGATION_PANE_TABS_QUERY_PARAM - ) as TPageNavigationPaneTab | null; - const activeTab: TPageNavigationPaneTab = navigationPaneQueryParam || "outline"; + const navigationPaneQueryParam = searchParams.get(PAGE_NAVIGATION_PANE_TABS_QUERY_PARAM); + const activeTab: TPageNavigationPaneTab = + navigationPaneQueryParam && + (PAGE_NAVIGATION_PANE_TAB_KEYS as readonly string[]).includes(navigationPaneQueryParam) + ? (navigationPaneQueryParam as TPageNavigationPaneTab) + : "outline";
71-79: Streamline handleTabChange (remove redundant alias).Minor cleanup; use value directly and reduce locals.
Apply this diff:
- (value: TPageNavigationPaneTab) => { - 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); - }, + (value: TPageNavigationPaneTab) => { + const isInfo = value === "info"; + const updatedRoute = updateQueryParams({ + paramsToAdd: { [PAGE_NAVIGATION_PANE_TABS_QUERY_PARAM]: value }, + paramsToRemove: !isInfo ? [PAGE_NAVIGATION_PANE_VERSION_QUERY_PARAM] : undefined, + }); + router.replace(updatedRoute); + },
74-79: Prefer replace over push for tab switches to avoid history spam.Tab changes are UI state, not page navigations. replace() keeps Back button behavior sane.
If you keep the current structure, minimally change push → replace:
- router.push(updatedRoute); + router.replace(updatedRoute);
84-90: Add an accessible name to the aside landmark.Provide an aria-label so screen readers can quickly identify this region.
Apply this diff:
- <aside + <aside + aria-label="Page navigation pane" className="flex-shrink-0 h-full flex flex-col bg-custom-background-100 pt-3.5 border-l border-custom-border-200 transition-all duration-300 ease-out"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
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(4 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)apps/web/core/components/pages/version/editor.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- apps/web/core/components/cycles/analytics-sidebar/issue-progress.tsx
- apps/web/core/components/modules/analytics-sidebar/issue-progress.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
apps/web/core/components/pages/navigation-pane/tabs-list.tsx (1)
packages/i18n/src/store/index.ts (1)
t(224-245)
apps/web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
apps/web/ce/components/pages/navigation-pane/tab-panels/empty-states/assets.tsx (1)
PageNavigationPaneAssetsTabEmptyState(7-26)
apps/web/core/components/pages/navigation-pane/tab-panels/root.tsx (2)
packages/propel/src/tabs/tabs.tsx (1)
TabsContent(96-96)apps/web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
PageNavigationPaneAssetsTabPanel(103-119)
apps/web/core/components/cycles/active-cycle/cycle-stats.tsx (2)
packages/hooks/src/use-local-storage.tsx (1)
useLocalStorage(24-55)packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)
apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx (4)
packages/hooks/src/use-local-storage.tsx (1)
useLocalStorage(24-55)packages/ui/src/tabs/tabs.tsx (1)
Tabs(29-88)packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx (3)
StateStatComponent(191-222)AssigneeStatComponent(75-127)LabelStatComponent(129-189)
apps/web/core/components/license/modal/card/base-paid-plan-card.tsx (2)
packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)packages/ui/src/billing/subscription.ts (1)
getSubscriptionBackgroundColor(35-73)
apps/web/core/components/core/image-picker-popover.tsx (2)
packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)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/pages/navigation-pane/root.tsx (3)
packages/propel/src/tabs/tabs.tsx (1)
Tabs(89-94)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/modules/analytics-sidebar/progress-stats.tsx (2)
packages/hooks/src/use-local-storage.tsx (1)
useLocalStorage(24-55)packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)
apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsx (3)
packages/types/src/analytics.ts (1)
TAnalyticsTabsBase(34-34)packages/ui/src/tabs/tabs.tsx (1)
Tabs(29-88)packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)
⏰ 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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps BAC0
🔇 Additional comments (14)
apps/web/core/components/pages/version/editor.tsx (1)
7-7: LGTM — import order only.No behavioral change; safe to merge.
apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx (3)
8-8: LGTM — Tabs import migration.Consistent with Propel Tabs API.
238-239: Typed tab keys — good.Prevents invalid persisted values at compile time.
253-256: LocalStorage key scoped by cycle — good.Prevents cross-cycle leakage.
apps/web/core/components/cycles/active-cycle/cycle-stats.tsx (2)
41-42: Exported tab type — good.Improves safety for callers and persisted values.
46-46: Typed localStorage — good.Keeps stored value aligned with valid tabs.
apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx (2)
252-256: LGTM on Tabs migration and typed localStorage.Controlled Tabs + typed
useLocalStorageis clean and resilient. Nice.Also applies to: 300-303, 306-345
235-237: Remove unused props from the public typeroundedTab and noBackground are declared only in the prop types and have no consumers; remove them.
Files:
- apps/web/core/components/modules/analytics-sidebar/progress-stats.tsx
- apps/web/core/components/cycles/analytics-sidebar/progress-stats.tsx
size?: "xs" | "sm"; - roundedTab?: boolean; - noBackground?: boolean; };apps/web/app/(all)/[workspaceSlug]/(projects)/analytics/[tabId]/page.tsx (1)
75-87: LGTM on controlled Tabs and header layout.Routing via
onValueChangeis straightforward and keeps URL/source of truth aligned.apps/web/core/components/pages/navigation-pane/tab-panels/assets.tsx (1)
111-117: LGTM — safer rendering and empty state inline.Optional chaining on
assetsListand inline empty-state are clear and safe.apps/web/core/components/pages/navigation-pane/tab-panels/root.tsx (1)
24-33: LGTM — TabsContent migration looks correct.Panels now align with value-based Tabs; keys and values match
ORDERED_PAGE_NAVIGATION_TABS_LIST.apps/web/core/components/pages/navigation-pane/root.tsx (2)
6-7: Propel Tabs import looks correct.Importing Tabs, TabsList, TabsTrigger, TabsContent from @plane/propel/tabs aligns with the new API.
108-116: Tabs integration looks correct.Controlled value/onValueChange pattern and children composition with TabsList/Content are consistent with Propel Tabs usage.
apps/web/core/components/pages/navigation-pane/tabs-list.tsx (1)
2-3: Propel Tabs imports are appropriate.Switch to TabsList/TabsTrigger matches the new component API.
…earch functionality
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/components/core/image-picker-popover.tsx (1)
123-124: Fix “stuck uploading” when workspaceSlug is undefined.Early-return leaves isImageUploading=true and disables the UI.
Apply:
- if (!workspaceSlug) return; + if (!workspaceSlug) { + setIsImageUploading(false); + setToast({ + title: "Missing workspace", + message: "Cannot upload without a workspace context.", + type: TOAST_TYPE.ERROR, + }); + return; + }
♻️ Duplicate comments (3)
apps/web/core/components/core/image-picker-popover.tsx (3)
233-241: Correct use of Unsplash URLs.Good fix switching to image.urls.{regular/small}; no more getFileURL mismatch.
199-224: Broken RHF Controller: field value never updates (search input is effectively read‑only).Wire Input to RHF’s field and remove the duplicate local form state.
Apply:
- <Controller - control={control} - name="search" - render={({ field: { value, ref } }) => ( - <Input - id="search" - name="search" - type="text" - onKeyDown={(e) => { - if (e.key === "Enter") { - e.preventDefault(); - setSearchParams(formData.search); - } - }} - value={value} - onChange={(e) => setFormData({ ...formData, search: e.target.value })} - ref={ref} - placeholder="Search for images" - className="w-full text-sm" - /> - )} - /> - <Button variant="primary" onClick={() => setSearchParams(formData.search)} size="sm"> - Search - </Button> + <Controller + control={control} + name="search" + render={({ field }) => ( + <> + <Input + id="search" + name="search" + type="text" + onKeyDown={(e) => { + if (e.key === "Enter") { + e.preventDefault(); + setSearchParams((field.value as string)?.trim() ?? ""); + } + }} + value={(field.value as string) ?? ""} + onChange={(e) => field.onChange(e.target.value)} + ref={field.ref} + placeholder="Search for images" + className="w-full text-sm" + /> + <Button + variant="primary" + onClick={() => setSearchParams((field.value as string)?.trim() ?? "")} + size="sm" + > + Search + </Button> + </> + )} + />And remove the now-unused local state:
- const [formData, setFormData] = useState({ - search: "", - });
181-182: Unsplash panel not scrollable; content can be clipped.Outer container uses overflow-hidden and Unsplash content lacks its own scrolling. Mirror the Images tab’s scrolling.
Apply:
- <TabsContent value="unsplash" className="pt-4"> + <TabsContent value="unsplash" className="pt-4 flex-1 h-full overflow-auto">(Optional alternative: revert container overflow)
- className="... md:w-[36rem] overflow-hidden flex-1" + className="... md:w-[36rem] overflow-auto flex-1"Also applies to: 197-200
🧹 Nitpick comments (5)
apps/web/core/components/core/image-picker-popover.tsx (5)
183-196: Make default tab resilient to conditional tabs.Select the first visible tab and remount Tabs when availability changes.
Apply:
- <Tabs defaultValue={tabOptions[0].key}> + <Tabs defaultValue={initialTab} key={initialTab}>Add near the SWR blocks:
// derive visible tabs and initial selection const availableTabs = React.useMemo(() => { return ta 5100 bOptions.filter((t) => { if (t.key === "unsplash") return !(unsplashError && !unsplashImages); if (t.key === "images") return !(projectCoverImages && projectCoverImages.length === 0); return true; }); }, [unsplashError, unsplashImages, projectCoverImages]); const initialTab = availableTabs[0]?.key ?? "upload";Please confirm @plane/propel/tabs remounts on key change as expected.
66-73: Show an error state for Unsplash instead of an infinite loader.When unsplashError is set, the UI still shows Loader.
Apply:
- const { data: unsplashImages, error: unsplashError } = useSWR( + const { data: unsplashImages, error: unsplashError, mutate: reloadUnsplash } = useSWR(- {unsplashImages ? ( - unsplashImages.length > 0 ? ( + {unsplashError ? ( + <div className="pt-7 flex flex-col items-center gap-2"> + <p className="text-xs text-custom-text-300">Could not load Unsplash images.</p> + <Button size="sm" variant="neutral-primary" onClick={() => reloadUnsplash()}> + Retry + </Button> + </div> + ) : unsplashImages ? ( + unsplashImages.length > 0 ? (Also applies to: 225-260
86-90: Restrict dropzone to a single file.Avoid accepting multiple files when only one is used.
Apply:
const { getRootProps, getInputProps, isDragActive, fileRejections } = useDropzone({ onDrop, accept: ACCEPTED_COVER_IMAGE_MIME_TYPES_FOR_REACT_DROPZONE, maxSize: MAX_FILE_SIZE, + multiple: false, });
238-240: Alt text null‑safe fallback for Unsplash images.Unsplash alt_description can be null.
Apply:
- alt={image.alt_description} + alt={image.alt_description ?? "Unsplash image"}
228-236: Avoid shadowing the state variable “image” in maps.Use distinct names for clarity.
Apply:
- {unsplashImages.map((image) => ( + {unsplashImages.map((photo) => ( <div - key={image.id} + key={photo.id} ... - onChange(image.urls.regular); + onChange(photo.urls.regular); ... - <img - src={image.urls.small} - alt={image.alt_description ?? "Unsplash image"} + <img + src={photo.urls.small} + alt={photo.alt_description ?? "Unsplash image"}- {projectCoverImages.map((image, index) => ( + {projectCoverImages.map((imgSrc, index) => ( <div - key={image} + key={imgSrc} ... - onChange(image); + onChange(imgSrc); ... - src={image} + src={imgSrc}Also applies to: 267-275
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/core/components/core/image-picker-popover.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/core/components/core/image-picker-popover.tsx (2)
packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)packages/constants/src/file.ts (2)
ACCEPTED_COVER_IMAGE_MIME_TYPES_FOR_REACT_DROPZONE(9-14)MAX_FILE_SIZE(1-1)
⏰ 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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
…ria-label to tabs list
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/components/core/image-picker-popover.tsx (1)
92-144: Bug: Upload can get stuck if workspaceSlug is missing.You set isImageUploading(true) before early-returning when workspaceSlug is falsy, leaving the UI disabled.
Apply:
const handleSubmit = async () => { if (!image) return; - setIsImageUploading(true); + if (!isProfileCover && !workspaceSlug) { + setToast({ + message: "Workspace not found.", + type: TOAST_TYPE.ERROR, + title: "Cannot upload image", + }); + return; + } + setIsImageUploading(true); const uploadCallback = (url: string) => { onChange(url); setIsImageUploading(false); setImage(null); setIsOpen(false); }; - if (isProfileCover) { + if (isProfileCover) { await fileService .uploadUserAsset( { entity_identifier: "", entity_type: EFileAssetType.USER_COVER, }, image ) .then((res) => uploadCallback(res.asset_url)) .catch((error) => { console.error("Error uploading user cover image:", error); setIsImageUploading(false); setToast({ message: error?.error ?? "The image could not be uploaded", type: TOAST_TYPE.ERROR, title: "Image not uploaded", }); }); } else { - if (!workspaceSlug) return; await fileService .uploadWorkspaceAsset( - workspaceSlug.toString(), + workspaceSlug.toString(), { entity_identifier: projectId?.toString() ?? "", entity_type: EFileAssetType.PROJECT_COVER, }, image ) .then((res) => uploadCallback(res.asset_url)) .catch((error) => { console.error("Error uploading project cover image:", error); setIsImageUploading(false); setToast({ message: error?.error ?? "The image could not be uploaded", type: TOAST_TYPE.ERROR, title: "Image not uploaded", }); }); } };
♻️ Duplicate comments (4)
apps/web/core/components/pages/navigation-pane/tabs-list.tsx (1)
12-18: Accessible tablist label added — good upgrade.The tab group now has a programmatic name. Matches prior feedback.
apps/web/core/components/core/image-picker-popover.tsx (3)
280-283: Fix validated — direct URL usage is correct.Using src={image} for project images resolves the earlier getFileURL concern.
197-263: Unsplash panel: handle errors and enable scrolling.
- Error state currently shows a loader; display an error message.
- Unsplash content isn’t scrollable while the container is overflow-hidden; add overflow to the panel.
Apply:
- <TabsContent value="unsplash" className="pt-4"> + {(() => { + const hasUnsplash = !(unsplashError && !unsplashImages); + if (!hasUnsplash) return null; + return ( + <TabsContent value="unsplash" className="pt-4 flex-1 h-full overflow-auto"> <div className="flex gap-x-2"> <Controller control={control} name="search" - render={({ field: { value, ref, onChange } }) => ( + render={({ field: { value, ref: fieldRef, onChange } }) => ( <Input id="search" name="search" type="text" onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); setSearchParams(value); } }} value={value} onChange={(e) => { onChange(e.target.value); setFormData({ ...formData, search: e.target.value }); }} - ref={ref} + ref={fieldRef} placeholder="Search for images" className="w-full text-sm" /> )} /> <Button variant="primary" onClick={() => setSearchParams(formData.search)} size="sm"> Search </Button> </div> - {unsplashImages ? ( + {unsplashError ? ( + <p className="pt-7 text-center text-xs text-red-600">Unable to fetch images.</p> + ) : unsplashImages ? ( unsplashImages.length > 0 ? ( <div className="grid grid-cols-4 gap-4"> {unsplashImages.map((image) => ( <div key={image.id} className="relative col-span-2 aspect-video md:col-span-1" onClick={() => { setIsOpen(false); onChange(image.urls.regular); }} > <img src={image.urls.small} - alt={image.alt_description} + alt={image.alt_description ?? "Unsplash image"} className="absolute left-0 top-0 h-full w-full cursor-pointer rounded object-cover" /> </div> ))} </div> ) : ( <p className="pt-7 text-center text-xs text-custom-text-300">No images found.</p> ) ) : ( <Loader className="grid grid-cols-4 gap-4"> <Loader.Item height="80px" width="100%" /> <Loader.Item height="80px" width="100%" /> <Loader.Item height="80px" width="100%" /> <Loader.Item height="80px" width="100%" /> <Loader.Item height="80px" width="100%" /> <Loader.Item height="80px" width="100%" /> <Loader.Item height="80px" width="100%" /> <Loader.Item height="80px" width="100%" /> </Loader> )} - </TabsContent> + </TabsContent> + ); + })()}
183-196: Default tab may target a non-rendered tab.defaultValue points to the first static option ("unsplash"), but that trigger can be conditionally hidden. Compute a safe default based on availability.
Apply:
- <Tabs defaultValue={tabOptions[0].key}> + {/* + Compute default tab from current availability. + hasUnsplash: hide when error and no data. + hasImages: show when loading (undefined) or non-empty. + */} + {(() => { + const hasUnsplash = !(unsplashError && !unsplashImages); + const hasImages = !projectCoverImages || projectCoverImages.length !== 0; + const defaultTab = hasUnsplash ? "unsplash" : hasImages ? "images" : "upload"; + return ( + <Tabs defaultValue={defaultTab}> + {/* TabsList and contents unchanged below */} +``` …and close the IIFE just before the closing </Tabs>: ```diff - </Tabs> + </Tabs> + ); + })()}
🧹 Nitpick comments (7)
apps/web/core/components/pages/navigation-pane/tabs-list.tsx (1)
12-12: Internationalize the aria-label.Prefer an i18n key over a hardcoded English string for SR users.
Apply:
- <TabsList aria-label="Page navigation tabs"> + <TabsList aria-label={t("page_navigation_pane.tabs.label")}>And add the key in locales (en) as "Page navigation tabs".
apps/web/core/components/core/image-picker-popover.tsx (6)
184-185: Add an accessible label to this tablist.Screen readers need a programmatic name for the image sources.
Apply:
- <TabsList> + <TabsList aria-label="Image sources">
86-90: Limit dropzone to a single file.The UI and state handle only one file; prevent multiple selections.
Apply:
const { getRootProps, getInputProps, isDragActive, fileRejections } = useDropzone({ onDrop, accept: ACCEPTED_COVER_IMAGE_MIME_TYPES_FOR_REACT_DROPZONE, maxSize: MAX_FILE_SIZE, + multiple: false, });
200-223: Minor: avoid shadowing outer ref name.Rename RHF’s field ref for clarity.
Apply:
- render={({ field: { value, ref, onChange } }) => ( + render={({ field: { value, ref: fieldRef, onChange } }) => ( ... - ref={ref} + ref={fieldRef}
224-227: Reduce duplicate state for search.You keep RHF field.value and a local formData.search. Consider using only RHF (watch/getValues) to drive setSearchParams to avoid drift.
240-244: Alt text fallback for Unsplash images.alt_description can be null; provide a fallback.
Apply:
- alt={image.alt_description} + alt={image.alt_description ?? "Unsplash image"}
349-353: Avoid leaking object URLs.URL.createObjectURL should be revoked to prevent memory leaks.
Add:
// state const [previewUrl, setPreviewUrl] = useState<string | null>(null); // effect useEffect(() => { if (!image) { setPreviewUrl(null); return; } const url = URL.createObjectURL(image); setPreviewUrl(url); return () => URL.revokeObjectURL(url); }, [image]);Then:
- <img src={URL.createObjectURL(image)} alt="Preview" ... /> + {previewUrl && <img src={previewUrl} alt="Preview" ... />}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/core/components/core/image-picker-popover.tsx(2 hunks)apps/web/core/components/pages/navigation-pane/tabs-list.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/core/components/core/image-picker-popover.tsx (2)
packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)packages/constants/src/file.ts (2)
ACCEPTED_COVER_IMAGE_MIME_TYPES_FOR_REACT_DROPZONE(9-14)MAX_FILE_SIZE(1-1)
⏰ 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: Analyze (javascript)
- GitHub Check: Build and lint web apps
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/core/components/core/image-picker-popover.tsx (1)
124-145: Blocking: Upload can get stuck when workspaceSlug is missingEarly return leaves isImageUploading=true and the UI disabled.
Apply:
- } else { - if (!workspaceSlug) return; + } else { + if (!workspaceSlug) { + setIsImageUploading(false); + setToast({ + title: "Workspace missing", + message: "Cannot upload without a workspace context.", + type: TOAST_TYPE.ERROR, + }); + return; + }
♻️ Duplicate comments (3)
apps/web/core/components/core/image-picker-popover.tsx (3)
209-227: Search value inconsistency between Enter and buttonMake both use the same source to avoid desync.
Minimal change:
- if (e.key === "Enter") { + if (e.key === "Enter") { e.preventDefault(); - setSearchParams(value); + setSearchParams((formData.search ?? "").trim()); }Recommended: remove formData mirror; use RHF only (e.g., useWatch({ control, name: "search" })) and read that in the button click.
325-331: Preview image: drop getFileURL and update Next/Image API; also prevent blob URL leaks
- getFileURL is unnecessary here and can break rendering if value is already an absolute URL (it is elsewhere in this component).
- layout/objectFit are deprecated in Next 13+. Use fill and className/object-cover.
- Revoke blob URLs to avoid memory leaks.
Apply:
- <Image - layout="fill" - objectFit="cover" - src={image ? URL.createObjectURL(image) : value ? (getFileURL(value) ?? "") : ""} - alt="image" - className="rounded-lg" - /> + <Image + fill + src={image ? previewUrl : (value ?? "")} + alt="Preview image" + className="rounded-lg object-cover" + />And add (outside this block):
// state + cleanup const [previewUrl, setPreviewUrl] = useState<string>(""); useEffect(() => { if (!image) { setPreviewUrl(""); return; } const url = URL.createObjectURL(image); setPreviewUrl(url); return () => URL.revokeObjectURL(url); }, [image]);
184-197: Default tab may point to a hidden tab; make default dynamic (or control the Tabs)Minimal fix:
- <Tabs defaultValue={tabOptions[0].key}> + {/* + If Unsplash fails and project images are empty, fall back to "upload". + */} + <Tabs + defaultValue={ + (!unsplashImages && unsplashError) + ? (projectCoverImages && projectCoverImages.length === 0 ? "upload" : "images") + : "unsplash" + } + >Recommended (stronger): control the selection to ensure the active tab is always visible.
🧹 Nitpick comments (4)
apps/web/core/components/core/image-picker-popover.tsx (4)
81-83: imagePickerRef is unusedRemove the ref and its assignment.
Also applies to: 181-183
241-246: Alt text may be null from Unsplash; add fallback- <img - src={image.urls.small} - alt={image.alt_description} + <img + src={image.urls.small} + alt={image.alt_description ?? "Unsplash image"}
345-350: File rejection UI assumes at least one errorDefensive read to avoid undefined access and show all messages:
- {fileRejections.length > 0 && ( - <p className="text-sm text-red-500"> - {fileRejections[0].errors[0].code === "file-too-large" - ? "The image size cannot exceed 5 MB." - : "Please upload a file in a valid format."} - </p> - )} + {fileRejections.length > 0 && ( + <div className="text-sm text-red-500 space-y-1"> + {fileRejections.flatMap((r) => r.errors).map((err, i) => ( + <p key={i}>{err.code === "file-too-large" ? "The image size exceeds the limit." : err.message}</p> + ))} + </div> + )}Optionally derive the displayed size from MAX_FILE_SIZE to keep text accurate.
317-323: “Edit” button has no actionEither wire it to open the file dialog (via useDropzone’s open) or remove it to avoid confusing UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/core/components/core/image-picker-popover.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/core/components/core/image-picker-popover.tsx (1)
packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)
⏰ 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). (10)
- GitHub Check: Build-Push Proxy Docker Image
- GitHub Check: Build-Push Space Docker Image
- GitHub Check: Build-Push Web Docker Image
- GitHub Check: Build-Push Live Collaboration Docker Image
- GitHub Check: Build-Push Admin Docker Image
- GitHub Check: Build-Push API Server Docker Image
- GitHub Check: Cursor Bugbot
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
apps/web/core/components/core/image-picker-popover.tsx (1)
200-221: RHF Controller wiring looks correct nowGood: value is driven by RHF; onChange calls field.onChange; local state is updated after.
|
@JayashTripathy , could you please follow the compound component approach as we discussed in our last conversation? |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
1 similar comment
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Actionable comments posted: 0
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/core/image-picker-popover.tsx (2)
124-145: Fix stuck “Uploading…” when workspaceSlug is missing.Early
returnleavesisImageUploadingtrue and no toast; UI gets stuck.- if (!workspaceSlug) return; + if (!workspaceSlug) { + setIsImageUploading(false); + setToast({ + title: "Workspace missing", + message: "Cannot upload cover without a workspace.", + type: TOAST_TYPE.ERROR, + }); + return; + }
83-89: Harden preview: guard empty drops, correct null-check, revoke Object URLs; also update Next/Image props.
acceptedFiles[0]can be undefined.image !== nulltreatsundefinedas present.URL.createObjectURLleaks without revoke.layout/objectFitare deprecated in Next/Image; usefill+object-cover.- const onDrop = useCallback((acceptedFiles: File[]) => { - setImage(acceptedFiles[0]); - }, []); + const onDrop = useCallback((acceptedFiles: File[]) => { + setImage(acceptedFiles[0] ?? null); + }, []);- {image !== null || (value && value !== "") ? ( + {(image != null) || (value && value !== "") ? ( <> <Image - layout="fill" - objectFit="cover" - src={image ? URL.createObjectURL(image) : value ? (getFileURL(value) ?? "") : ""} + fill + src={image ? previewUrl ?? "" : value ? (getFileURL(value) ?? "") : ""} alt="image" - className="rounded-lg" + className="rounded-lg object-cover" /> </>Additional code (outside this range):
// imports import React, { useState, useRef, useCallback, useEffect } from "react"; // state + cleanup const [previewUrl, setPreviewUrl] = useState<string | null>(null); useEffect(() => { if (!image) { setPreviewUrl(null); return; } const url = URL.createObjectURL(image); setPreviewUrl(url); return () => URL.revokeObjectURL(url); }, [image]);Also applies to: 325-334
♻️ Duplicate comments (3)
apps/web/core/components/core/image-picker-popover.tsx (3)
59-61: Unify search state (use RHF only) to avoid desync between RHF and local formData.Drop
formData, drive the input with RHF, and read the current value viauseWatch(or place Button inside the Controller). This removes inconsistent sources of truth.- const [formData, setFormData] = useState({ - search: "", - });- <Controller - control={control} - name="search" - render={({ field: { value, ref, onChange } }) => ( + <Controller + control={control} + name="search" + defaultValue="" + render={({ field }) => ( <Input id="search" name="search" type="text" onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); - setSearchParams(value); + setSearchParams((field.value as string) ?? ""); } }} - value={value} - onChange={(e) => { - onChange(e.target.value); - setFormData({ ...formData, search: e.target.value }); - }} - ref={ref} + value={(field.value as string) ?? ""} + onChange={(e) => field.onChange(e.target.value)} + ref={field.ref} placeholder="Search for images" className="w-full text-sm" /> )} /> - <Button variant="primary" onClick={() => setSearchParams(formData.search)} size="sm"> + <Button + variant="primary" + onClick={() => setSearchParams((control?._formValues?.search as string) ?? "")} + size="sm" + > Search </Button>Alternatively (preferred), add at top and use it:
// imports import { Control, Controller, useWatch } from "react-hook-form"; // inside component const searchValue = useWatch({ control, name: "search", defaultValue: "" });Then:
- <Button variant="primary" onClick={() => setSearchParams(formData.search)} size="sm"> + <Button variant="primary" onClick={() => setSearchParams(searchValue ?? "")} size="sm">Also applies to: 200-226, 226-229
184-197: Make Tabs controlled and auto-fallback to an available tab (prevents blank panel when a tab is hidden).Right now
defaultValue="unsplash"can point to a hidden/invalid tab (e.g., Unsplash error), leaving the UI stuck. Control the Tabs value and recompute a valid fallback when availability changes.Apply this diff in-place:
- <Tabs defaultValue={tabOptions[0].key}> + <Tabs value={activeTab} onValueChange={setActiveTab}>Additional code (outside this range): define availability and controlled state, and auto-correct when data changes.
// imports: add useEffect import React, { useState, useRef, useCallback, useEffect } from "react"; ... // after SWR hooks const showUnsplash = !(unsplashError && !unsplashImages); const showImages = !(projectCoverImages && projectCoverImages.length === 0); const computeDefaultTab = () => (showUnsplash ? "unsplash" : showImages ? "images" : "upload"); const [activeTab, setActiveTab] = useState<string>(computeDefaultTab()); useEffect(() => { const available = [showUnsplash && "unsplash", showImages && "images", "upload"].filter(Boolean) as string[]; if (!available.includes(activeTab)) setActiveTab(available[0]); // eslint-disable-next-line react-hooks/exhaustive-deps }, [showUnsplash, showImages]);
198-266: Unsplash error shows infinite loader; surface an error state and keep panel scrollable.When
unsplashErroris set andunsplashImagesis falsy, the UI renders the Loader forever. Also, make the content area scrollable.- <TabsContent value="unsplash" className="pt-4 h-full overflow-hidden"> + <TabsContent value="unsplash" className="pt-4 h-full flex-1 overflow-auto"> <div className="flex flex-col h-full"> <div className="flex gap-x-2"> ... - {unsplashImages ? ( + {unsplashError ? ( + <p className="pt-7 text-center text-xs text-red-500">Couldn’t load images. Try again.</p> + ) : unsplashImages ? ( unsplashImages.length > 0 ? ( <div className="grid grid-cols-4 gap-4 flex-1 overflow-y-auto pt-4">
🧹 Nitpick comments (3)
apps/web/core/components/core/image-picker-popover.tsx (3)
234-247: Use buttons for selectable thumbnails (keyboard/a11y).Clickable divs aren’t keyboard-focusable; switch to buttons.
- <div + <button + type="button" key={image.id} className="relative col-span-2 aspect-video md:col-span-1" onClick={() => { setIsOpen(false); onChange(image.urls.regular); }} - > + > <img src={image.urls.small} alt={image.alt_description} className="absolute left-0 top-0 h-full w-full cursor-pointer rounded object-cover" /> - </div> + </button>- <div + <button + type="button" key={image} className="relative col-span-2 aspect-video md:col-span-1" onClick={() => { setIsOpen(false); onChange(image); }} - > + > <img src={image} alt={`Project cover ${index + 1}`} className="absolute left-0 top-0 h-full w-full cursor-pointer rounded object-cover" /> - </div> + </button>Also applies to: 273-287
166-174: Avoid mixing HeadlessUI Popover’s internal state with customisOpen.You’re using
Popover.Buttonbut controlling visibility via customisOpenand an outside click hook. Either:
- Use Popover’s own state (remove
isOpen,static, and custom outside click), or- Replace Popover with a plain positioned container driven by
isOpen.Also applies to: 176-183
81-83: Remove unusedimagePickerRef.It’s assigned but never read.
- const imagePickerRef = useRef<HTMLDivElement>(null); ... - <div - ref={imagePickerRef} + <divAlso applies to: 180-183
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/core/components/core/image-picker-popover.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/core/components/core/image-picker-popover.tsx (1)
packages/propel/src/tabs/tabs.tsx (4)
Tabs(89-94)TabsList(96-96)TabsTrigger(96-96)TabsContent(96-96)
⏰ 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). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/web/core/components/core/image-picker-popover.tsx (2)
330-331: VerifygetFileURL(value)handles absolute URLs.If
valueis already an absolute URL (e.g., Unsplash/asset_url), double‑prefixing can break images. ConfirmgetFileURLis idempotent for absolute URLs.
14-14: Good migration to Propel Tabs API.Compound API usage (Tabs, TabsList, TabsTrigger, TabsContent) looks consistent with @plane/propel.
Ensure this file’s usage matches the compound export signature in
packages/propel/src/tabs/tabs.tsx(Tabs.List/Trigger/Content names).
…actor-propel-tabs
Description
Replaced tabs implementation everywhere with propel/tabs
Type of Change
Screenshots and Media (if applicable)
analytics
cycle
modules
image picker
Summary by CodeRabbit
New Features
Refactor