Staging #660
Staging #660
Conversation
- Add D3 dependencies and types - Implement advanced force simulation parameters in ForceGraph - Adjust link and charge forces for better graph layout - Add collision force to prevent node overlap - Improve graph centering and node distribution
Fix #651 fix force engine
Fix #654 Bump dependencies version
Fix #652 fix search in table view
Fix #649 improve graph zoom-to-fit padding calculation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several improvements across multiple components. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
…ledge graph population
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/components/ForceGraph.tsx (1)
82-112: Excellent improvements to force graph layout!The force parameters have been well-tuned for better visualization:
- Link distances adapt based on node connections
- Collision detection prevents node overlap
- Center force keeps the graph centered
- Charge force improves node distribution
Consider memoizing the force calculations if performance becomes an issue with large graphs.
app/components/TableComponent.tsx (2)
115-125: Extract nested search logic into a separate function for better maintainability.The recursive search implementation is correct but could be more maintainable and potentially more performant.
Consider extracting the nested search logic into a separate utility function:
+const searchInValue = (value: any, searchTerm: string): boolean => { + if (typeof value === "object") { + return Object.values(value).some(v => + typeof v === "object" + ? searchInValue(v, searchTerm) + : v?.toString().toLowerCase().includes(searchTerm.toLowerCase()) + ); + } + return value?.toString().toLowerCase().includes(searchTerm.toLowerCase()); +}; rows.filter((row) => !search || row.cells.some(cell => - cell.value && ( - typeof cell.value === "object" - ? Object.values(cell.value).some(value => - typeof value === "object" - ? Object.values(value).some(val => val.toString().toLowerCase().includes(search.toLowerCase())) - : value?.toString().toLowerCase().includes(search.toLowerCase()) - ) - : cell.value.toString().toLowerCase().includes(search.toLowerCase()) - ) + cell.value && searchInValue(cell.value, search) ))
127-132: Document the purpose of the data-id attribute.The conditional data-id attribute logic is correct, but its purpose and usage should be documented for better maintainability.
Add a comment explaining why the data-id is only set for string values:
<TableRow onMouseEnter={() => setHover(`${i}`)} onMouseLeave={() => setHover("")} + // Only set data-id for string values to ensure valid DOM attribute values data-id={typeof row.cells[0].value === "string" ? row.cells[0].value : undefined} key={i} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
app/components/ForceGraph.tsx(3 hunks)app/components/TableComponent.tsx(2 hunks)app/graph/GraphView.tsx(2 hunks)app/graph/toolbar.tsx(2 hunks)lib/utils.ts(1 hunks)package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (3)
app/graph/toolbar.tsx (1)
7-7: LGTM! Good refactoring of zoom functionality.The zoom-to-fit functionality has been properly refactored to use the new utility function, improving code modularity.
Also applies to: 43-45
app/graph/GraphView.tsx (1)
10-10: LGTM! Good integration of zoom utility.The zoom-to-fit functionality has been properly integrated into the query handling logic.
Also applies to: 211-213
package.json (1)
56-56:❓ Verification inconclusive
Verify compatibility with the major version update of lucide-react.
The update from ^0.301.0 to ^0.475.0 is a significant jump that could introduce breaking changes.
Please check the changelog and migration guide:
🌐 Web query:
What are the breaking changes in lucide-react between version 0.301.0 and 0.475.0?💡 Result:
Between lucide-react versions 0.301.0 and 0.475.0, key breaking changes include:
1. ESM Migration and Naming Convention Changes
- Icon exports changed from
camelCasetoPascalCasefor React components (e.g.,home→Home)[3][5]- Requires updating icon imports:
// Old (pre-ESM) import { home } from 'lucide-react'; // New (post-ESM) import { Home } from 'lucide-react';2. Package Structure Changes
- Introduced stricter ESM module resolution via
"exports"field inpackage.json[5]- May break projects using non-standard import paths like:
// No longer supported import Icon from 'lucide-react/dist/esm/icons/Home';3. Dynamic Icon Handling
- Replaced
dynamicIconImportswith newDynamicIconcomponent[6]- New usage pattern:
import { DynamicIcon } from 'lucide-react/dynamic'; // Instead of dynamic imports <DynamicIcon name="camera" />4. SVG String Formatting
- Added whitespace around
<svg>tags in raw icon strings[3]- Requires trimming for some SVG manipulation libraries:
// Before passing to SVG.js etc. const cleanedSvg = rawSvgString.trim();Compatibility Considerations
These changes primarily affect:
- Projects using dynamic icon name resolution[3]
- Custom build pipelines importing internal modules[5]
- Applications relying on exact SVG string formatting[3]
For most static icon usage (direct component imports), upgrades should be straightforward with simple naming convention updates[6].
Citations:
- 1: https://bundlephobia.com/package/lucide-react-native
- 2: https://www.npmjs.com/package/lucide-react?activeTab=versions
- 3: Breaking changes between
lucide-static@307andlucide-static@308+(package moved to ESM) lucide-icons/lucide#2145- 4: https://bestofjs.org/projects/lucide
- 5: https://github.com/lucide-icons/lucide/releases
- 6: https://lucide.dev/guide/packages/lucide-react
- 7: https://lucide.dev/icons/wrap-text
- 8: https://www.npmjs.com/package/lucide-react
- 9: https://www.jsdelivr.com/package/npm/lucide
- 10: https://www.jsdelivr.com/package/npm/lucide-react-native
Verify compatibility with the major version update of lucide-react.
The update from ^0.301.0 to ^0.475.0 introduces several breaking changes that may affect your project:
- Renamed Icon Exports: Icon components are now exported in PascalCase rather than camelCase. Ensure any icon imports (e.g.,
import { home } from 'lucide-react';) are updated to the new format (e.g.,import { Home } from 'lucide-react';).- Package Structure Modifications: The package now uses stricter ESM module resolution via the
"exports"field. Verify that you are not relying on unofficial import paths (such as importing directly from internal directories likelucide-react/dist/esm/icons/Home).- Dynamic Icon Handling: If you use dynamic imports for icons, note that the previous
dynamicIconImportsmechanism has been replaced by a newDynamicIconcomponent. Update your usage accordingly.- SVG String Formatting: There’s now additional whitespace around
<svg>elements in raw icon strings. If your project processes SVGs programmatically, consider trimming these strings where necessary.Please review your code for these potential impact areas and update your imports and usage patterns as needed to ensure compatibility.
Fix #661 Remove unused flask app
Fix types
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/components/ForceGraph.tsx (1)
81-111: Extract force parameters as named constants.The force parameters should be extracted as named constants to improve maintainability and make adjustments easier.
Apply this diff to improve the implementation:
+const LINK_DISTANCE = 30; +const CHARGE_STRENGTH = -30; +const CHARGE_MAX_DISTANCE = 150; +const COLLISION_RADIUS_MULTIPLIER = 1.5; +const CENTER_FORCE_STRENGTH = 0.05; + useEffect(() => { if (!chartRef.current) return; // Adjust force parameters for better graph layout const linkForce = chartRef.current.d3Force('link'); if (linkForce) { linkForce - .distance(() => 30) + .distance(() => LINK_DISTANCE) .strength((link: any) => 1 / Math.min( graph.Elements.nodes.filter(n => n.id === link.source.id).length, graph.Elements.nodes.filter(n => n.id === link.target.id).length )); } // Adjust charge force for node repulsion const chargeForce = chartRef.current.d3Force('charge'); if (chargeForce) { chargeForce - .strength(-30) - .distanceMax(150); + .strength(CHARGE_STRENGTH) + .distanceMax(CHARGE_MAX_DISTANCE); } // Add collision force to prevent node overlap chartRef.current.d3Force('collision', - d3.forceCollide(NODE_SIZE * 1.5) + d3.forceCollide(NODE_SIZE * COLLISION_RADIUS_MULTIPLIER) ); // Center force to keep graph centered const centerForce = chartRef.current.d3Force('center'); if (centerForce) { - centerForce.strength(0.05); + centerForce.strength(CENTER_FORCE_STRENGTH); } }, [chartRef, graph.Elements.nodes])app/schema/SchemaView.tsx (1)
214-251: Remove commented-out Cytoscape-specific code.The commented-out
handleSetCategoryfunction contains Cytoscape-specific code that is no longer relevant after migrating to Force Graph. Consider removing this code to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/components/ForceGraph.tsx(3 hunks)app/graph/GraphView.tsx(3 hunks)app/graph/toolbar.tsx(2 hunks)app/schema/SchemaView.tsx(2 hunks)e2e/logic/POM/settingsUsersPage.ts(2 hunks)e2e/tests/settingsConfig.spec.ts(0 hunks)lib/utils.ts(2 hunks)
💤 Files with no reviewable changes (1)
- e2e/tests/settingsConfig.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/graph/toolbar.tsx
- app/graph/GraphView.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (2)
lib/utils.ts (1)
95-107: Improve type safety and error handling in handleZoomToFit.The function has several areas for improvement:
- Magic numbers should be named constants
- Direct DOM querying is fragile
- Missing error handling when canvas is not found
- Minimum padding value mentioned in the comment is not enforced
Apply this diff to improve the implementation:
+const ZOOM_DURATION_MS = 1000; +const ZOOM_PADDING_RATIO = 0.1; +const MIN_ZOOM_PADDING = 40; + export function handleZoomToFit(chartRef?: MutableRefObject<ForceGraphMethods<Node, Link> | undefined>) { const chart = chartRef?.current if (chart) { // Get canvas dimensions const canvas = document.querySelector('.force-graph-container canvas') as HTMLCanvasElement; - if (!canvas) return; + if (!canvas) { + console.warn('Canvas element not found for zoom-to-fit operation'); + return; + } // Calculate padding as 10% of the smallest canvas dimension, with minimum of 40px const minDimension = Math.min(canvas.width, canvas.height); - const padding = minDimension * 0.1 - chart.zoomToFit(1000, padding) + const padding = Math.max(minDimension * ZOOM_PADDING_RATIO, MIN_ZOOM_PADDING); + chart.zoomToFit(ZOOM_DURATION_MS, padding) } }e2e/logic/POM/settingsUsersPage.ts (1)
23-25: LGTM! The XPath selector is now more flexible.The updated XPath expression correctly targets any button within the specified table cell, making the selector more resilient to changes in the button's position within the cell.
and place three dots when description is to long
…rDB/falkordb-browser into show-graphs-name-on-delete
Fix #664 show the graphs names on delete
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/TableComponent.tsx (1)
152-159: 🛠️ Refactor suggestionReuse the search logic for JSONTree expansion.
The JSONTree expansion logic duplicates the filtering implementation. This could lead to maintenance issues if the search logic needs to be updated.
Extract the search logic into a reusable function:
+const searchInValue = (value: any, searchTerm: string): boolean => { + if (typeof value === "object") { + return Object.values(value).some(v => + typeof v === "object" + ? searchInValue(v, searchTerm) + : v?.toString().toLowerCase().includes(searchTerm.toLowerCase()) + ); + } + return value?.toString().toLowerCase().includes(searchTerm.toLowerCase()); +}; <JSONTree key={search} - shouldExpandNodeInitially={() => - search !== "" && Object.values(cell.value as object).some(value => - typeof value === "object" - ? Object.values(value as object).some(val => - val.toString().toLowerCase().includes(search.toLowerCase()) - ) - : value?.toString().toLowerCase().includes(search.toLowerCase()))} + shouldExpandNodeInitially={() => search !== "" && searchInValue(cell.value, search)} keyPath={[headers[j]]}
🧹 Nitpick comments (1)
app/components/ui/combobox.tsx (1)
99-101: Enhance toast message readability.The current implementation directly joins graph names with commas. For better readability, consider formatting the list properly.
- description: `The graph(s) ${opts.join(", ")} have been deleted successfully`, + description: opts.length === 1 + ? `The graph "${opts[0]}" has been deleted successfully` + : `The following graphs have been deleted successfully: ${opts.join('", "')}"`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/DialogComponent.tsx(1 hunks)app/components/TableComponent.tsx(4 hunks)app/components/ui/combobox.tsx(3 hunks)
🔇 Additional comments (4)
app/components/ui/combobox.tsx (2)
150-157: LGTM! Good layout improvements.The flex layout and overflow handling changes improve the component's responsiveness and usability:
- Using
flex-colfor proper vertical layout- Setting
max-h-[90dvh]to prevent overflow- Adding
grow overflow-hiddento table for proper space utilization
162-172: LGTM! Enhanced user feedback.Good improvement to the delete confirmation dialog:
- Added max width constraint for better readability
- Including specific graph names in the confirmation message helps prevent accidental deletions
app/components/TableComponent.tsx (2)
39-42: LGTM! Good component flexibility.Adding the
classNameprop allows for better component customization and reusability.
116-126: LGTM! Enhanced search functionality.The improved filtering logic now properly handles nested objects and provides more comprehensive search results.
Fix coderabbitai comments
Summary by CodeRabbit
New Features
Chores