poc for content position controlled by selected items#1144
poc for content position controlled by selected items#1144kodiakhq[bot] merged 4 commits intomainfrom
Conversation
WalkthroughThis update introduces enhancements to the Select component system, notably adding dynamic offset positioning for dropdowns and a new story for portal-less rendering. Several context and prop types are extended to support these features. Minor code cleanups, such as removing extraneous whitespace and logging, are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SelectRoot
participant SelectPrimitiveRoot
participant SelectPrimitiveItem
App->>SelectRoot: Render with offsetValue
SelectRoot->>SelectPrimitiveRoot: Pass offsetValue prop
SelectPrimitiveRoot->>SelectPrimitiveItem: Provide context with selectedItemRef, setOffsetPositionValue
SelectPrimitiveItem->>SelectPrimitiveRoot: Assign ref if item is selected
Note over SelectPrimitiveRoot: useEffect updates offset based on selectedIndex and dropdown height
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 5
🧹 Nitpick comments (7)
src/components/ui/Select/stories/Select.stories.tsx (1)
34-34: Consider renaming the story for clarity.The name
BasicPortalis misleading since this story specifically demonstrates usage without a portal. Consider renaming toBasicWithoutPortalorBasicNoPortalto better reflect its purpose.-export const BasicPortal = () => { +export const BasicWithoutPortal = () => {src/components/ui/Select/fragments/SelectContent.tsx (2)
16-16: Remove debugging console.log.The console.log statement should be removed before merging to production.
- console.log(i);
10-20: Consider more robust selected item detection.The current logic relies on
aria-selected="true"being present on mount, which may not always be reliable. Consider using the component's value/defaultValue props instead of DOM inspection.- useEffect(() => { - if (!contentRef.current) return; - - for (let i = 0; i < contentRef.current?.children.length; i++) { - if (contentRef.current?.children[i].getAttribute('aria-selected') === 'true') { - console.log(i); - setSelectedId(i+1); - } - } - },[]) + useEffect(() => { + // Consider using value/defaultValue from Select context instead + // of DOM inspection for more reliable selected item detection + if (!contentRef.current) return; + + for (let i = 0; i < contentRef.current?.children.length; i++) { + if (contentRef.current?.children[i].getAttribute('aria-selected') === 'true') { + setSelectedId(i + 1); // Consider if this should be zero-based + break; // Exit loop once selected item is found + } + } + }, [])src/components/ui/Select/fragments/SelectRoot.tsx (3)
1-1: Remove unused imports.The
useCallbackanduseEffectimports are not used in this component.-import React, { useState, useCallback, useEffect } from 'react'; +import React, { useState } from 'react';
11-11: Remove debugging console.log.The console.log statement should be removed before merging to production.
- console.log(selectedId)
20-20: Consider making the offset multiplier configurable.The hardcoded
40pixel offset should be configurable to support different item heights or design requirements.+const ITEM_HEIGHT = 40; // or make this a prop + - offsetValue={-(40*selectedId)} + offsetValue={selectedId ? -(ITEM_HEIGHT * selectedId) : 0}src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
23-23: Fix formatting issue in parameter destructuring.There's a missing space before
offsetValuein the parameter list.-function SelectPrimitiveRoot({ children, className, value, name, defaultValue = '', onValueChange, onClickOutside = () => {}, placement = 'bottom-start',offsetValue, ...props }: SelectPrimitiveRootProps) { +function SelectPrimitiveRoot({ children, className, value, name, defaultValue = '', onValueChange, onClickOutside = () => {}, placement = 'bottom-start', offsetValue, ...props }: SelectPrimitiveRootProps) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ui/Select/contexts/SelectRootContext.tsx(1 hunks)src/components/ui/Select/fragments/SelectContent.tsx(2 hunks)src/components/ui/Select/fragments/SelectRoot.tsx(2 hunks)src/components/ui/Select/stories/Select.stories.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/components/ui/Select/fragments/SelectRoot.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(9-13)
src/components/ui/Select/fragments/SelectContent.tsx (1)
src/components/ui/Select/contexts/SelectRootContext.tsx (1)
SelectRootContext(9-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/components/ui/Select/fragments/SelectContent.tsx (1)
17-17: Clarify the index calculation logic.The
i+1calculation seems arbitrary. Consider adding a comment explaining why the index is incremented, or if this should be justifor zero-based indexing.Can you verify if this should be 1-based indexing? The offset calculation in
SelectRoot.tsxuses-(40*selectedId), so understanding the intended indexing is crucial for correct positioning.src/components/ui/Select/fragments/SelectRoot.tsx (1)
20-20:⚠️ Potential issueFix null handling in offset calculation.
When
selectedIdisnull(initial state), the calculation-(40*selectedId)results inNaN, which could cause positioning issues.- offsetValue={-(40*selectedId)} + offsetValue={selectedId ? -(40 * selectedId) : 0}Likely an incorrect or invalid review comment.
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
17-17: Good addition of the offsetValue prop.The new optional
offsetValueprop is appropriately typed and aligns with the POC objective of controlling content position.
|
|
||
| function SelectContent({ customRootClass, children, position = 'popper', ...props }: any) { | ||
| const { rootClass } = useContext(SelectRootContext); | ||
| const { rootClass, setSelectedId } = useContext(SelectRootContext); |
There was a problem hiding this comment.
Fix type mismatch with context.
The code extracts setSelectedId from SelectRootContext, but this property is commented out in the context type definition (SelectRootContext.tsx lines 5-6). This will cause TypeScript errors.
- const { rootClass, setSelectedId } = useContext(SelectRootContext);
+ const { rootClass, setSelectedId } = useContext(SelectRootContext) as any; // TODO: Fix context typesAlternatively, update the context type definition to include these properties.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { rootClass, setSelectedId } = useContext(SelectRootContext); | |
| const { rootClass, setSelectedId } = useContext(SelectRootContext) as any; // TODO: Fix context types |
🤖 Prompt for AI Agents
In src/components/ui/Select/fragments/SelectContent.tsx at line 7, the code
destructures setSelectedId from SelectRootContext, but setSelectedId is
commented out in the SelectRootContext type definition (around lines 5-6 in
SelectRootContext.tsx), causing a type mismatch. To fix this, update the
SelectRootContext type definition to include setSelectedId and rootClass
properties with their correct types, ensuring the context provides these values
and TypeScript recognizes them.
|
|
||
| return ( | ||
| <SelectRootContext.Provider value={{ rootClass }}> | ||
| <SelectRootContext.Provider value={{ rootClass, selectedId, setSelectedId }}> |
There was a problem hiding this comment.
Fix context type mismatch.
The context value includes selectedId and setSelectedId, but these are commented out in the SelectRootContextType interface, causing TypeScript errors.
This needs to be coordinated with the context type definition in SelectRootContext.tsx.
🤖 Prompt for AI Agents
In src/components/ui/Select/fragments/SelectRoot.tsx at line 14, the context
value passed to SelectRootContext.Provider includes selectedId and
setSelectedId, but these properties are commented out in the
SelectRootContextType interface, causing a type mismatch. To fix this, update
the SelectRootContextType interface in SelectRootContext.tsx to include
selectedId and setSelectedId with the correct types, ensuring the context value
matches the interface definition and resolves the TypeScript errors.
| // selectedId: '', | ||
| // setSelectedId: () => {} |
There was a problem hiding this comment.
Update default context values.
If the properties are uncommented in the type definition, also uncomment and fix the default values:
export const SelectRootContext = React.createContext<SelectRootContextType>({
rootClass: '',
- // selectedId: '',
- // setSelectedId: () => {}
+ selectedId: null,
+ setSelectedId: () => {}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // selectedId: '', | |
| // setSelectedId: () => {} | |
| export const SelectRootContext = React.createContext<SelectRootContextType>({ | |
| rootClass: '', | |
| selectedId: null, | |
| setSelectedId: () => {} | |
| }); |
🤖 Prompt for AI Agents
In src/components/ui/Select/contexts/SelectRootContext.tsx at lines 11 to 12,
the default context values for selectedId and setSelectedId are commented out.
Uncomment these lines and provide appropriate default values: set selectedId to
an empty string and setSelectedId to a no-op function to match the type
definition and ensure the context has valid defaults.
| // selectedId: string; | ||
| // setSelectedId: React.Dispatch<React.SetStateAction<string>> |
There was a problem hiding this comment.
Resolve context type inconsistency.
The selectedId and setSelectedId properties are commented out in the type definition but are actively used in SelectRoot.tsx and SelectContent.tsx. This creates TypeScript errors throughout the codebase.
Either uncomment these properties to support the new functionality:
interface SelectRootContextType {
rootClass: string;
-// selectedId: string;
-// setSelectedId: React.Dispatch<React.SetStateAction<string>>
+ selectedId: number | null;
+ setSelectedId: React.Dispatch<React.SetStateAction<number | null>>
}Or remove their usage from other components if this approach is being abandoned.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // selectedId: string; | |
| // setSelectedId: React.Dispatch<React.SetStateAction<string>> | |
| // src/components/ui/Select/contexts/SelectRootContext.tsx | |
| interface SelectRootContextType { | |
| rootClass: string; | |
| selectedId: number | null; | |
| setSelectedId: React.Dispatch<React.SetStateAction<number | null>>; | |
| } |
🤖 Prompt for AI Agents
In src/components/ui/Select/contexts/SelectRootContext.tsx around lines 5 to 6,
the selectedId and setSelectedId properties are commented out in the context
type definition but are used in SelectRoot.tsx and SelectContent.tsx, causing
TypeScript errors. To fix this, uncomment these two properties in the type
definition to align with their usage, ensuring type consistency across
components.
| const { refs, floatingStyles, context: floatingContext } = Floater.useFloating({ | ||
| middleware: [Floater.offset(offsetValue)], | ||
| open: isOpen, | ||
| onOpenChange: setIsOpen, | ||
| placement | ||
| placement, | ||
| }); |
There was a problem hiding this comment.
Handle undefined offsetValue and optimize middleware array.
The current implementation has two issues:
offsetValueis optional but passed directly toFloater.offset(), which may not handleundefinedgracefully- The middleware array is recreated on every render
+ const middleware = React.useMemo(() => {
+ const middlewares = [];
+ if (offsetValue !== undefined) {
+ middlewares.push(Floater.offset(offsetValue));
+ }
+ return middlewares;
+ }, [offsetValue]);
+
const { refs, floatingStyles, context: floatingContext } = Floater.useFloating({
- middleware: [Floater.offset(offsetValue)],
+ middleware,
open: isOpen,
onOpenChange: setIsOpen,
placement,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { refs, floatingStyles, context: floatingContext } = Floater.useFloating({ | |
| middleware: [Floater.offset(offsetValue)], | |
| open: isOpen, | |
| onOpenChange: setIsOpen, | |
| placement | |
| placement, | |
| }); | |
| const middleware = React.useMemo(() => { | |
| const middlewares: Floater.Middleware[] = []; | |
| if (offsetValue !== undefined) { | |
| middlewares.push(Floater.offset(offsetValue)); | |
| } | |
| return middlewares; | |
| }, [offsetValue]); | |
| const { refs, floatingStyles, context: floatingContext } = Floater.useFloating({ | |
| middleware, | |
| open: isOpen, | |
| onOpenChange: setIsOpen, | |
| placement, | |
| }); |
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx around lines 39
to 44, the offsetValue passed to Floater.offset may be undefined, which can
cause issues, and the middleware array is recreated on every render causing
unnecessary re-renders. Fix this by conditionally including
Floater.offset(offsetValue) in the middleware array only if offsetValue is
defined, and memoize the middleware array using React.useMemo to avoid
recreating it on each render.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (1)
44-48:⚠️ Potential issueHandle undefined offsetValue and optimize middleware array.
The offsetValue handling issue from the previous review still needs to be addressed.
+ const middleware = React.useMemo(() => { + const middlewares = []; + if (offsetPositionValue !== undefined) { + middlewares.push(Floater.offset(offsetPositionValue)); + } + return middlewares; + }, [offsetPositionValue]); + const { refs, floatingStyles, context: floatingContext } = Floater.useFloating({ - middleware: [Floater.offset(offsetPositionValue)], + middleware, open: isOpen, onOpenChange: setIsOpen, placement, });
🧹 Nitpick comments (3)
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1)
17-17: Remove trailing whitespace.- +src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx (2)
79-80: Extract position calculation for clarity.The position calculation could be clearer with better variable naming.
- const position = (selectedIndex) * (floatingElement.clientHeight / floatingElement.children.length); - setOffsetPositionValue(-position); + const itemHeight = floatingElement.clientHeight / floatingElement.children.length; + const offset = selectedIndex * itemHeight; + setOffsetPositionValue(-offset);
63-81: Consider edge cases in positioning logic.The current implementation might have issues with:
- Empty children array (division by zero)
- Dynamic content that changes the number of items
- Items with varying heights
Would you like me to help implement a more robust positioning system that handles these edge cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/ui/Select/fragments/SelectContent.tsx(2 hunks)src/components/ui/Select/fragments/SelectRoot.tsx(2 hunks)src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx(1 hunks)src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ui/Select/fragments/SelectRoot.tsx
- src/components/ui/Select/fragments/SelectContent.tsx
🔇 Additional comments (2)
src/core/primitives/Select/fragments/SelectPrimitiveItem.tsx (1)
15-15: LGTM!The implementation correctly assigns the
selectedItemRefonly to the selected item, which aligns with the positioning control objective.Also applies to: 25-25
src/core/primitives/Select/contexts/SelectPrimitiveContext.tsx (1)
20-21: LGTM!The context type extensions are well-defined and support the positioning control feature.
| const index = children.indexOf(selectedItemRef.current); | ||
|
|
||
| setSelectedIndex(index + 1); | ||
| }, [refs.floating.current, selectedItemRef?.current]); |
There was a problem hiding this comment.
Fix dependency array syntax.
The dependency arrays use incorrect syntax with .current. React expects the ref object itself, not the current value.
- }, [refs.floating.current, selectedItemRef?.current]);
+ }, [refs.floating, selectedItemRef]);And similarly:
- }, [isOpen, selectedIndex, refs.floating.current]);
+ }, [isOpen, selectedIndex, refs.floating]);Also applies to: 81-81
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx at lines 71 and
81, the dependency arrays incorrectly include `.current` from ref objects. React
expects the ref objects themselves as dependencies, not their `.current` values.
Remove `.current` from the dependency arrays so that only the ref objects (e.g.,
refs.floating and selectedItemRef) are included.
| const children = Array.from(floatingElement.children); | ||
| const index = children.indexOf(selectedItemRef.current); | ||
|
|
||
| setSelectedIndex(index + 1); |
There was a problem hiding this comment.
Fix incorrect index calculation.
Adding 1 to the index seems incorrect. Array indices start at 0, so this would make the first item have index 1 instead of 0, potentially causing off-by-one errors in the position calculation.
- setSelectedIndex(index + 1);
+ setSelectedIndex(index);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setSelectedIndex(index + 1); | |
| setSelectedIndex(index); |
🤖 Prompt for AI Agents
In src/core/primitives/Select/fragments/SelectPrimitiveRoot.tsx at line 70, the
code adds 1 to the index when setting selectedIndex, which causes an off-by-one
error. Remove the addition of 1 so that selectedIndex is set directly to the
index value, ensuring correct zero-based indexing.
kotAPI
added
the
automerge
2 participants
#1127
currently not working with portal but its a poc, improvements and recommendations are welcomed
Recording.2025-06-07.023408.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Refactor