Fix #1262 Refactor selection handling in graph components to use selectedElemen… #1263
Conversation
…ts array instead of individual selectedElement. Update related functions and props across ForceGraph, GraphView, PaginationList, Selector, and Toolbar components for improved state management.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces single-item selection props with array-based multi-selection across graph and schema components; updates selection APIs and call sites to use Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PaginationList as PaginationList (UI)
participant Selector as Selector (State)
participant Storage as localStorage
rect rgb(200,220,240)
Note over User,PaginationList: Mark items for deletion
User->>PaginationList: Right-click history item
PaginationList->>Selector: onClick(label, evt='rightclick')
activate Selector
Selector->>Selector: toggle deleteElements[index]
Selector-->>PaginationList: isDeleteSelected(item) -> true/false
deactivate Selector
PaginationList->>User: Render highlight (deleteSelected)
end
rect rgb(240,220,200)
Note over User,Storage: Confirm and perform deletion
User->>Selector: Click Delete (or Delete All)
activate Selector
Selector->>Selector: compute remaining queries (filter by deleteElements)
Selector->>Storage: write updated history
Selector->>Selector: clear deleteElements, refresh state
deactivate Selector
Selector->>PaginationList: Re-render list
PaginationList->>User: Updated history shown
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
app/graph/Selector.tsx (1)
293-313: Line 312 has inverted filtering logic that will delete the wrong queries.The condition
!deleteElements.some((removeIndex) => historyQuery.queries[removeIndex].timestamp !== query.timestamp)uses!==(not equal) when it should use===(equal). This causes the filter to keep queries that should be deleted and remove queries that should be kept.Change to:
!deleteElements.some((removeIndex) => historyQuery.queries[removeIndex].timestamp === query.timestamp)
🧹 Nitpick comments (2)
app/components/PaginationList.tsx (1)
289-296: Consider the maintainability of synthetic event types.Creating a synthetic event with a custom
"rightclick"type via type assertion works but is non-standard. Consider documenting this pattern or using a discriminated union for event types to improve type safety at consumer sites.Example alternative approach:
-onClick={(e) => { - onClick(text, e) -}} -onContextMenu={(e) => { - e.preventDefault() - const syntheticEvent = { - ...e, - type: "rightclick" as const - } as typeof e & { type: "rightclick" } - onClick(text, syntheticEvent) +onClick={(e) => onClick(text, { event: e, type: 'click' as const })} +onContextMenu={(e) => { + e.preventDefault() + onClick(text, { event: e, type: 'rightclick' as const }) }}app/graph/Selector.tsx (1)
427-455: Consider adding confirmation for "Delete All".The "Delete All" button (lines 438-454) permanently removes all query history. Consider adding a confirmation dialog to prevent accidental data loss, similar to the pattern used for element deletion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/ForceGraph.tsx(2 hunks)app/components/PaginationList.tsx(10 hunks)app/graph/GraphView.tsx(2 hunks)app/graph/Selector.tsx(11 hunks)app/graph/page.tsx(6 hunks)app/graph/toolbar.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/components/ForceGraph.tsxapp/graph/page.tsxapp/graph/Selector.tsxapp/graph/toolbar.tsxapp/graph/GraphView.tsxapp/components/PaginationList.tsx
🧬 Code graph analysis (5)
app/components/ForceGraph.tsx (1)
app/api/graph/model.ts (1)
Link(68-84)
app/graph/page.tsx (1)
app/graph/DataPanel.tsx (1)
DataPanel(25-207)
app/graph/Selector.tsx (2)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)app/graph/toolbar.tsx (1)
Toolbar(31-333)
app/graph/toolbar.tsx (1)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)
app/components/PaginationList.tsx (2)
lib/utils.ts (1)
cn(91-93)app/api/graph/model.ts (1)
Query(17-26)
🔇 Additional comments (13)
app/components/PaginationList.tsx (2)
32-37: LGTM! Good refactoring.The
getItemClassNamehelper effectively consolidates class selection logic, making the code more maintainable and following DRY principles.
237-237: LGTM! Consistent event handling.Event parameters are consistently passed through the component, and the
deleteSelectedstate is properly propagated to rendering functions.Also applies to: 262-262, 272-272, 286-288
app/graph/toolbar.tsx (2)
16-16: LGTM! Clean API migration.The prop signature cleanly migrates from single-element to multi-element selection, maintaining type safety with the array-based setter.
Also applies to: 35-35
109-109: LGTM! Consistent multi-element selection usage.Single elements are properly wrapped in arrays, and the DeleteElement visibility check correctly uses
selectedElements.length !== 0.Also applies to: 319-319
app/components/ForceGraph.tsx (3)
408-418: LGTM! Standard multi-select pattern implemented.The right-click selection logic follows familiar UX patterns:
- Right-click alone: replace selection with single element
- Ctrl+right-click: toggle element in/out of selection
420-423: LGTM! Thoughtful deselection logic.The early return when
ctrlKeyis held prevents accidental deselection during multi-select operations, enhancing the user experience.
425-426: LGTM! Rendering logic correctly adapted.Both link and node rendering properly check membership in the
selectedElementsarray using.some(), maintaining visual feedback for selection state.Also applies to: 465-467
app/graph/page.tsx (3)
66-66: LGTM! Proper array state management.The
selectedElementsstate is correctly initialized as an empty array and consistently cleared when transitioning between panels or modes.Also applies to: 95-95, 184-184
230-241: LGTM! Good adapter pattern.The
handleSetSelectedElementfunction serves as a clean adapter, wrapping single elements into the array format while maintaining the existing single-element API for internal usage.
310-313: Verify the UX for multi-selection with DataPanel.When multiple elements are selected, the DataPanel displays only the last element (
selectedElements[selectedElements.length - 1]). Consider whether this behavior is intuitive for users or if it should show a different element (e.g., first) or prevent opening the data panel entirely when multiple elements are selected.app/graph/GraphView.tsx (1)
21-22: LGTM! Clean prop threading.The migration to array-based selection is consistently applied throughout the component, with proper prop threading to Toolbar and ForceGraph, and appropriate state reset on graph changes.
Also applies to: 90-91, 129-130, 217-218
app/graph/Selector.tsx (2)
119-119: LGTM! Appropriate state for deletion tracking.Using an array of indices to track elements marked for deletion is a clean approach for batch deletion operations.
386-401: LGTM! Consistent multi-select interaction pattern.The onClick handler follows the same interaction model as ForceGraph (right-click to select, Ctrl+right-click to multi-select), providing a consistent UX across the application.
…eleteSelected prop and remove selectedElement state. Update related logic to manage selectedElements array for improved selection handling.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/graph/Selector.tsx (1)
292-311: Verify the filtered queries update logic.Line 310 filters
filteredQueriesby comparing timestamps, but this assumes timestamps are unique identifiers. If two queries can have the same timestamp, this could fail. Additionally, the double negation (!deleteElements.some(... !== ...)) is harder to reason about.Consider simplifying the logic:
- setFilteredQueries(current => current.filter(query => !deleteElements.some((removeIndex) => historyQuery.queries[removeIndex].timestamp !== query.timestamp))) + setFilteredQueries(current => current.filter(query => + !deleteElements.some((removeIndex) => + historyQuery.queries[removeIndex].timestamp === query.timestamp + ) + ))Actually, looking more carefully, the current logic appears incorrect. It should check for equality (
===), not inequality (!==). The double negation makes it confusing.
🧹 Nitpick comments (1)
app/schema/_page.tsx (1)
65-80: Consider renaming and clarifying append-only behavior.The function name
handleSetSelectedElement(singular) is misleading since it now manages an array. Additionally, line 67 always appends without checking for duplicates or toggling, which could lead to duplicate elements inselectedElements.Consider:
- Renaming to
handleToggleElementorhandleSetSelectedElementsfor clarity- Adding deduplication logic if needed
- const handleSetSelectedElement = useCallback((el: Node | Link | undefined) => { + const handleToggleSelectedElement = useCallback((el: Node | Link | undefined) => { if (el) { - setSelectedElements(prev => [...prev, el]) + setSelectedElements(prev => { + const exists = prev.some(e => e.id === el.id) + return exists ? prev : [...prev, el] + }) setIsAddNode(false) setIsAddEdge(false) } else { setSelectedElements([]) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/PaginationList.tsx(11 hunks)app/graph/Selector.tsx(11 hunks)app/schema/SchemaView.tsx(1 hunks)app/schema/_page.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/schema/SchemaView.tsxapp/schema/_page.tsxapp/graph/Selector.tsxapp/components/PaginationList.tsx
🧬 Code graph analysis (2)
app/schema/_page.tsx (1)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)
app/components/PaginationList.tsx (3)
lib/utils.ts (1)
cn(91-93)app/api/graph/model.ts (1)
Query(17-26)app/components/ui/Button.tsx (1)
Props(11-21)
⏰ 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: Setup and cache dependencies
🔇 Additional comments (5)
app/components/PaginationList.tsx (1)
32-37: LGTM: Clean abstraction with the helper function.The
getItemClassNamehelper improves readability by consolidating the classification logic with proper precedence: selected > deleteSelected > hover > default.app/schema/SchemaView.tsx (1)
58-59: LGTM: Clean migration to multi-element selection.The
useEffectproperly resetsselectedElements(array) when the schema changes, removing the old single-element logic. This aligns well with the broader multi-select refactoring across the codebase.app/graph/Selector.tsx (2)
425-453: LGTM: Delete functionality is well-structured.The Delete and Delete All buttons provide clear UX with helpful tooltips. The Delete button is properly disabled when no elements are selected, and Delete All when there are no queries.
36-37: LGTM: Clean separation of Graph vs Schema props.The type-safe union approach using conditional types ensures that Graph and Schema modes have the correct props. The
selectedElementsis properly constrained to Schema mode only.Also applies to: 58-59
app/schema/_page.tsx (1)
252-260: No action required — the guard is sufficient.Line 252's conditional check
if (selectedElements.length !== 0)properly guards the array access at line 255. The code executes synchronously within a single render cycle, so the state cannot change between the check and access. The array is initialized as[]and only updated viasetSelectedElements(), making the guard adequate.
…s across components for improved selection handling. Update related functions in ForceGraph, GraphView, DataPanel, and SchemaView to align with new prop type.
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 (2)
app/schema/_page.tsx (1)
65-79: Bug: truthy check on array always passes for empty arrays.
if (el)will betruefor an empty array[]. This means the panel will expand even when selection is cleared.const handleSetSelectedElements = useCallback((el: (Node | Link)[] = []) => { setSelectedElements(el) - if (el.length !== 0) { + if (el.length > 0) { setIsAddNode(false) setIsAddEdge(false) } const currentPanel = panelRef.current if (!currentPanel) return - if (el) currentPanel.expand() + if (el.length > 0) currentPanel.expand() else currentPanel.collapse() }, [])app/graph/page.tsx (1)
283-295: Redundant selection clearing logic.Line 286 already clears selection with
setSelectedElements([]), then lines 288-289 do additional clearing. The logic after line 286 is unreachable in terms of effect since selection is already empty.setRelationships(graph.removeLinks(selectedElements.map((element) => element.id))) setData({ ...graph.Elements }) fetchCount() - setSelectedElements([]) - if (panel === "data") handleSetSelectedElements() - else setSelectedElements([]) + handleSetSelectedElements() toast({ title: "Success", description: `${selectedElements.length > 1 ? "Elements" : "Element"} deleted`, }) - }, [selectedElements, graph, setData, fetchCount, panel, handleSetSelectedElements, toast, setIndicator]) + }, [selectedElements, graph, setData, fetchCount, handleSetSelectedElements, toast, setIndicator])
🧹 Nitpick comments (3)
app/schema/DataPanel.tsx (1)
324-331: Minor inconsistency: prefersetObject()oversetObject(undefined)for clarity.Line 57 uses
setObject()while line 327 usessetObject(undefined). Both work, but using the same pattern improves readability.- onClick={() => setObject(undefined)} + onClick={() => setObject()}app/components/ForceGraph.tsx (1)
425-426: Consider type-safe link identification.Using
el.sourceto distinguish links from nodes works but relies on structural typing. A more explicit check could improve clarity.- const isLinkSelected = (link: Link) => (selectedElements.length > 0 && selectedElements.some(el => el.id === link.id && el.source)) + const isLinkSelected = (link: Link) => (selectedElements.length > 0 && selectedElements.some(el => el.id === link.id && 'source' in el))app/graph/GraphView.tsx (1)
133-133: Consider using a type guard for improved clarity.The runtime check
selectedElements.every(e => !!e.labels)works correctly to verify both selected elements are Nodes. However, a type guard function would be more explicit and type-safe:const isNode = (el: Node | Link): el is Node => 'labels' in el; // Then use: selectedElements.every(isNode)This makes the intent clearer and provides better TypeScript inference.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/ForceGraph.tsx(3 hunks)app/graph/GraphView.tsx(3 hunks)app/graph/page.tsx(8 hunks)app/schema/DataPanel.tsx(2 hunks)app/schema/SchemaView.tsx(2 hunks)app/schema/_page.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
Review React components for proper hooks usage, accessibility, and performance.
Files:
app/schema/DataPanel.tsxapp/graph/GraphView.tsxapp/schema/_page.tsxapp/schema/SchemaView.tsxapp/components/ForceGraph.tsxapp/graph/page.tsx
🧬 Code graph analysis (6)
app/schema/DataPanel.tsx (1)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)
app/graph/GraphView.tsx (1)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)
app/schema/_page.tsx (2)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)app/schema/DataPanel.tsx (1)
DataPanel(30-731)
app/schema/SchemaView.tsx (1)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)
app/components/ForceGraph.tsx (1)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)
app/graph/page.tsx (2)
app/api/graph/model.ts (2)
Node(55-66)Link(68-84)app/graph/DataPanel.tsx (1)
DataPanel(25-207)
⏰ 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: Setup and cache dependencies
🔇 Additional comments (16)
app/schema/DataPanel.tsx (2)
23-28: LGTM - API updated for multi-selection support.The prop signature correctly allows optional array parameter, enabling callers to clear selection by calling
setObject()with no arguments.
53-59: LGTM - Escape handler updated correctly.Calling
setObject()without arguments aligns with the new optional parameter signature to clear selection.app/components/ForceGraph.tsx (4)
17-37: LGTM - Props interface updated for multi-selection.The
setSelectedElementssignature correctly uses optional parameter pattern for clearing selection.
408-418: LGTM - Multi-select with Ctrl+click implemented correctly.Toggle behavior with
ctrlKeyand single-selection fallback work as expected for multi-element selection UX.
420-423: LGTM - Background click clears selection appropriately.Early return when
ctrlKeyis pressed or selection is empty prevents unnecessary state updates.
465-467: LGTM - Node selection highlighting updated.Linewidth calculation correctly uses
selectedElements.some()to check node selection status.app/schema/_page.tsx (2)
59-63: LGTM - Panel sizing logic updated for array-based selection.
250-259: LGTM - DataPanel receives last selected element.Using
selectedElements[selectedElements.length - 1]to show the most recently selected element is a reasonable UX choice.app/schema/SchemaView.tsx (2)
14-29: LGTM - Props interface aligned with multi-selection pattern.
57-59: LGTM - Selection reset on schema change.Clearing selection when
schema.Idchanges prevents stale references to elements from a previous schema.app/graph/page.tsx (3)
230-240: LGTM - Selection handler with panel management.Logic correctly opens data panel when elements are selected and closes when cleared.
307-315: LGTM - DataPanel integration with multi-selection.Using the last element of
selectedElementsfor display andhandleSetSelectedElements()for close is consistent with the new pattern.
391-394: LGTM - Collapse handler clears selection.app/graph/GraphView.tsx (3)
20-39: Props interface correctly updated for multi-selection.The refactoring from single
selectedElementtoselectedElementsarray is well-structured. The optional parameter insetSelectedElementsprovides flexibility for clearing selections or setting multiple elements.
89-91: Selection clearing logic looks good.The useEffect properly clears the selection when the graph changes, using an empty array to reset
selectedElements. The dependency array is correct.
217-218: ForceGraph integration updated correctly.The multi-selection props are properly passed to ForceGraph, maintaining consistency with the refactoring across components.
| <ResizableHandle | ||
| withHandle | ||
| onMouseUp={() => isCollapsed && handleSetSelectedElement(undefined)} | ||
| onMouseUp={() => isCollapsed && handleSetSelectedElements(undefined)} | ||
| className={cn("ml-6 w-0", isCollapsed && "hidden")} | ||
| /> |
There was a problem hiding this comment.
Type inconsistency: pass empty array instead of undefined.
handleSetSelectedElements(undefined) conflicts with the signature (el: (Node | Link)[] = []). Use handleSetSelectedElements() or handleSetSelectedElements([]) for clarity.
- onMouseUp={() => isCollapsed && handleSetSelectedElements(undefined)}
+ onMouseUp={() => isCollapsed && handleSetSelectedElements()}📝 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.
| <ResizableHandle | |
| withHandle | |
| onMouseUp={() => isCollapsed && handleSetSelectedElement(undefined)} | |
| onMouseUp={() => isCollapsed && handleSetSelectedElements(undefined)} | |
| className={cn("ml-6 w-0", isCollapsed && "hidden")} | |
| /> | |
| <ResizableHandle | |
| withHandle | |
| onMouseUp={() => isCollapsed && handleSetSelectedElements()} | |
| className={cn("ml-6 w-0", isCollapsed && "hidden")} | |
| /> |
🤖 Prompt for AI Agents
In app/schema/_page.tsx around lines 330 to 334, the call
handleSetSelectedElements(undefined) conflicts with its signature which expects
an array (el: (Node | Link)[] = []); replace the undefined with either no
argument (handleSetSelectedElements()) or an explicit empty array
(handleSetSelectedElements([])) to match the expected type; update the onMouseUp
handler to call one of these to ensure type consistency.
…ts array instead of individual selectedElement. Update related functions and props across ForceGraph, GraphView, PaginationList, Selector, and Toolbar components for improved state management.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.