Chart theming and rendering#74
Conversation
Comprehensive plan for integrating tldraw SDK to visualize Laravel and Rails database schemas as interactive ERD diagrams on an infinite canvas in the viz column.
Implement Entity Relationship Diagram visualization using tldraw SDK: - Add tldraw (4.2.3) and dagre for infinite canvas and auto-layout - Create ERD parser service supporting: - Laravel migrations and Eloquent models - Rails schema.rb and ActiveRecord models - Mermaid ERD syntax import - Create custom EntityShapeUtil for tldraw shapes - Add ERDCanvasPanel component with: - Framework detection (Laravel/Rails badge) - Auto-layout using Dagre algorithm - Pan/zoom infinite canvas via tldraw - Register 'erd-canvas' as new VizPanelType - Add IPC handlers: get-erd-schema, detect-erd-framework, parse-mermaid-erd - Add comprehensive CSS styles for ERD entities
…nvas-38Tye ERD infinite canvas
Co-authored-by: peter <peter@icehouseventures.co.nz>
|
Cursor Agent can help with this pull request. Just |
- Add file-graph option to chart selector dropdown and settings panel - Add VizPanelType for file-graph - Use VizHeader component for consistent header with chart switcher - Fix directory color to use --bg-hover for better visibility
- Resolve conflicts between FileGraph and ERD features - Both visualization panels now available in chart menu - Add erd-canvas option to SettingsPanel for consistency
|
@claude review |
|
Claude finished @peterjthomson's task in 2m 20s —— View job Code Review Complete ✓Reviewing PR #74: Chart theming and rendering Review Checklist:
SummaryThis PR successfully refactors chart theming to use CSS theme tokens and splits FileGraph's treemap layout into a renderer-agnostic helper. It also adds comprehensive ERD visualization features. The code quality is generally good, but there are several areas that need attention. 🟢 Strengths1. Excellent Theming RefactoringThe migration from hardcoded colors to CSS variables (
This ensures all visualizations adapt to the user's selected theme automatically. 2. FileGraph Treemap RefactoringThe extraction of layout logic into
3. Column Drag FixThe fix in 4. ERD ImplementationThe ERD feature is comprehensive with multiple renderers (tldraw, React Flow, JSON), proper loading states, and framework detection (Laravel/Rails). 🟡 Issues & Recommendations1. Security: CSP Changes Need Review (MODERATE PRIORITY)File: The CSP was modified to allow content="default-src 'self' https://cdn.tldraw.com; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data: res: blob: https://cdn.tldraw.com; connect-src 'self' https://cdn.tldraw.com;"Issues:
Recommendations:
2. Missing Error Boundaries (LOW PRIORITY)Files: All chart components None of the visualization components have error boundaries. If a chart crashes (e.g., malformed data, calculation error), it could break the entire panel. Recommendation: 3. Type Safety: Missing Null Checks (LOW PRIORITY)File: masterY: y + MASTER_HEIGHT / 2 - 20,The Recommendation: 4. Performance: ResizeObserver Without Debouncing (LOW PRIORITY)File: The ResizeObserver triggers a re-render on every resize event, which could cause performance issues during window resizing. Recommendation: const timeoutRef = useRef<NodeJS.Timeout>()
useEffect(() => {
if (!containerRef.current) return
const observer = new ResizeObserver((entries) => {
clearTimeout(timeoutRef.current)
timeoutRef.current = setTimeout(() => {
const entry = entries[0]
if (entry) {
setDimensions({ width: entry.contentRect.width, height: entry.contentRect.height })
}
}, 100)
})
// ... rest of code
}, [])5. Code Duplication: Ridge Path Generation (LOW PRIORITY)File: The Recommendation: 6. Language Color Mapping (INFORMATIONAL)File: The language color mapping now uses CSS variables, but it's not clear where the Recommendation: 7. Magic Numbers (LOW PRIORITY)Multiple files have hardcoded dimensions and spacing values:
Recommendation: 🔵 Nice Touches
📦 Dependency AdditionsThe PR adds several significant dependencies:
Question: Were bundle size impacts assessed? The app already has 50+ dependencies. 📊 Test CoverageI don't see any tests added for the new functionality. Consider adding:
✅ Final RecommendationAPPROVE with minor suggestions. This is a solid PR that achieves its goals:
The code is production-ready, but I'd recommend addressing the CSP/CDN issue before merging if possible. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // Syntax: ENTITY1 cardinality--cardinality ENTITY2 : label | ||
| // Cardinality patterns: || (one), |o/o| (zero-or-one), |{/}| (one-or-more), o{/}o (zero-or-more) | ||
| // Use symmetric pattern for both sides to handle all valid Mermaid notations | ||
| const cardinalityPattern = '(?:\\|[|o]|o[|{]|[{}][|o]?)' |
There was a problem hiding this comment.
Mermaid cardinality pattern missing |{ notation
Medium Severity
The regex pattern for Mermaid cardinality doesn't match |{ (one-or-more). The pattern (?:\\|[|o]|o[|{]|[{}][|o]?) only matches || and |o for pipe-based patterns, missing |{. According to the comment on line 617, |{ and }| both represent "one-or-more". Mermaid ERD relationships using |{-- notation won't be parsed, causing missing relationships in the diagram.
| from: { entity: tableName, cardinality: 'many', attribute: attr.name }, | ||
| to: { entity: attr.foreignKey.table, cardinality: 'one', attribute: attr.foreignKey.column }, | ||
| type: 'identifying', | ||
| }) |
There was a problem hiding this comment.
Duplicate relationship IDs for multiple foreign keys
Medium Severity
When a Rails schema table has multiple foreign keys to the same target table, the relationship ID doesn't include the attribute name, causing ID collisions. For example, if orders table has both customer_id and salesperson_id pointing to users, both relationships get ID orders_users. When tldraw creates arrows using these IDs (line 124 of erdUtils.ts), duplicate IDs cause one relationship arrow to be lost or overwritten, making the diagram incomplete.
3 participants
Split FileGraph treemap layout into a renderer-agnostic helper and updated chart styles to match the user-selected app theme.
This PR refactors the FileGraph treemap layout to align with the analysis/render pattern from master and updates the styling of various charts (GitGraph, TechTree, FileGraph, ContributorChart) to use theme-derived color palette tokens, ensuring they adapt to the user's active app theme.
Note
Adds a new ERD visualization and aligns visualizations with app theming.
ERDviz panel with renderers: tldraw infinite canvas, React Flow graph, and JSON inspectorCanvasRenderercase forerd-canvas, shared layout and stylesFileGraphtreemap to renderer-agnosticsquarifyhelper; updates language color mapping and legendindex.htmlWritten by Cursor Bugbot for commit d5c9cef. This will update automatically on new commits. Configure here.