[PAI-963] feat: enhance CustomSelect component with context for dropdown management #8202
Conversation
WalkthroughIntroduces a DropdownContext and refactors custom select to memoize open/close/toggle callbacks, make options close the dropdown via context and requestAnimationFrame, remove the static Combobox.Options flag, and add a rounded style to custom button variants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
pratapalakshmi
changed the title
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui/src/dropdowns/custom-select.tsx (2)
3-3: Context wiring for dropdown close is solid; consider stricter default.Using
DropdownContextto sharecloseDropdownintoOptionkeeps the close behavior nicely centralized, and the() => voidtype keeps call sites simple. If you want to catch incorrect usage ofCustomSelect.Optionoutside ofCustomSelect, consider defaulting the context tonull(or throwing in the default function) and warning/throwing in development instead of silently no‑oping. This would surface mis-wiring earlier at runtime without affecting the happy path here.Also applies to: 16-18, 145-148
147-153: Option-level requestAnimationFrame close works; you can avoid redundant closes.Using
requestAnimationFrameinhandleMouseDownis a good way to ensure the dropdown closes after Headless UI has processed the click. With the current wiring, a click on a new option will close via bothCombobox’sonChangeandhandleMouseDown, which is harmless but redundant.If you want to trim that duplication while still closing when re-clicking an already-selected option, you could pass
selectedinto the handler and only force-close in that case, e.g.:- const handleMouseDown = useCallback(() => { - // Close dropdown for both new and already-selected options. - requestAnimationFrame(() => closeDropdown()); - }, [closeDropdown]); + const handleMouseDown = useCallback( + (selected: boolean) => { + // New selections already close via Combobox.onChange; only force-close + // when re-clicking the currently selected option. + if (selected) { + requestAnimationFrame(() => closeDropdown()); + } + }, + [closeDropdown] + ); ... - {({ selected }) => ( - <div - onMouseDown={handleMouseDown} + {({ selected }) => ( + <div + onMouseDown={() => handleMouseDown(selected)} className="flex items-center justify-between gap-2 w-full" >Not required, but slightly tightens the state update path.
Also applies to: 168-171, 174-174
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/dropdowns/custom-select.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
packages/ui/src/dropdowns/custom-select.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
packages/ui/src/dropdowns/custom-select.tsx
🧬 Code graph analysis (1)
packages/ui/src/dropdowns/custom-select.tsx (3)
packages/ui/src/hooks/use-dropdown-key-down.tsx (1)
useDropdownKeyDown(12-31)packages/ui/src/utils/classname.tsx (1)
cn(5-5)packages/ui/src/dropdowns/helper.tsx (1)
ICustomSelectItemProps(93-97)
⏰ 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 (3)
packages/ui/src/dropdowns/custom-select.tsx (3)
48-53: useCallback memoization for open/close/toggle looks correct.
openDropdown,closeDropdown, andtoggleDropdownhave the right dependency sets and give stable identities to pass intouseDropdownKeyDown,useOutsideClickDetector, and the context provider. This should avoid stale refs while keeping re-renders minimal. No changes needed.Also applies to: 57-61
63-77: Provider + Combobox + portal/outside-click integration is coherent.Wrapping
ComboboxinDropdownContext.Providerand closing via theonChangehandler keeps both keyboard and pointer selection flows collapsing the menu consistently, whiledata-prevent-outside-clickonCombobox.OptionsletsuseOutsideClickDetectortreat the portalled panel as “inside” even though it’s rendered underdocument.body. Moving popper refs/styles onto the inner container div is also fine since that’s the actual positioned element.Also applies to: 116-116, 140-142
83-85: Rounded styling on the custom button aligns the triggers.Adding
roundedto the custom button’s classes brings its shape in line with the default button variant, which should reduce visual inconsistencies between the two entry points.
…pdown options rendering
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui/src/dropdowns/custom-select.tsx (2)
63-141: Two open states (localisOpen+ Combobox internal) – please sanity‑check keyboard interactionsWith
isOpengating the portal and Headless UI’s ownopenstate controlling whether<Combobox.Options>renders (now thatstaticis removed), the dropdown effectively has two sources of truth. Mouse flows are well covered viaonChange+ the option’s close handler, but for keyboard‑only interactions (e.g., Arrow keys + Enter/Escape, or re‑selecting the already‑selected option) it’s possible forisOpenand the Combobox’s internal open state to drift out of sync, which could confuseuseDropdownKeyDownlogic or leave the portal wrapper mounted around a closed options list.I’d recommend explicitly testing those keyboard scenarios; if you see any inconsistency, consider consolidating to a single open state (deriving
isOpenfrom Combobox’sopenor moving all close behavior into Combobox callbacks) instead of maintaining both in parallel.
145-175: Option close-on-interaction is good, but queuedrequestAnimationFramecan update after unmountUsing
DropdownContextplus anonMouseDownhandler withrequestAnimationFrameis a nice way to ensure the dropdown closes even when the user clicks an already‑selected option, without racing Combobox’s own selection logic. One edge case to be aware of: if the parentonChangecausesCustomSelectto unmount (e.g., navigation or conditional rendering) during the same interaction, the queuedrequestAnimationFramecallback can still fire and callcloseDropdown, which would attempt a state update on an unmounted component.To harden this, you could either:
- Track an
isMountedref insideCustomSelectand early‑return incloseDropdownwhen unmounted, or- Store the RAF id in a ref inside
OptionandcancelAnimationFrameit in a cleanup effect.Separately, you may want
DropdownContextto default tonulland guard inOption(throw or log) when used outsideCustomSelect, so incorrect usage is caught early rather than silently no‑oping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/dropdowns/custom-select.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,mts,cts}
📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)
**/*.{ts,tsx,mts,cts}: Useconsttype parameters for more precise literal inference in TypeScript 5.0+
Use thesatisfiesoperator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicitisreturn types in filter/check functions
UseNoInfer<T>utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing inswitch(true)blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacyexperimentalDecorators
Useusingdeclarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Usewith { type: "json" }for import attributes; avoid deprecatedassertsyntax (TypeScript 5.3/5.8+)
Useimport typeexplicitly when importing types to ensure they are erased during compilation, respectingverbatimModuleSyntaxflag
Use.ts,.mts,.ctsextensions inimport typestatements (TypeScript 5.2+)
Useimport type { Type } from "mod" with { "resolution-mode": "import" }for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize newSetmethods likeunion,intersection, etc., when available (TypeScript 5.5+)
UseObject.groupBy/Map.groupBystandard methods for grouping instead of external libraries (TypeScript 5.4+)
UsePromise.withResolvers()for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted,toSpliced,with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields viasuperin classes (TypeScript 5....
Files:
packages/ui/src/dropdowns/custom-select.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-09T20:42:31.843Z
Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.
Applied to files:
packages/ui/src/dropdowns/custom-select.tsx
⏰ 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/ui/src/dropdowns/custom-select.tsx (1)
16-56: Context wiring and dropdown state helpers look solidUsing a simple
DropdownContextwith memoizedopenDropdown/closeDropdown/toggleDropdownaroundisOpenandreferenceElementkeeps visibility and focus management centralized, and the dependency lists look correct for bothuseDropdownKeyDownanduseOutsideClickDetector. I don’t see any correctness issues in this setup.
4 participants
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.