Cursor/Chart Theming And Rendering 2b1e#75
Conversation
Reconcile the FileGraph feature with the new analytics/conveyor architecture while bringing the branch up to date with master.
Co-authored-by: peter <peter@icehouseventures.co.nz>
- 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
- Integrate Code Graph (AST-based) feature from master - Both Code Graph and Code Map (FileGraph) now available in viz menu - Add codegraph option to SettingsPanel - Resolve CSS conflicts by keeping master styles and appending FileGraph styles
|
@claude review |
|
Claude finished @peterjthomson's task in 1m 54s —— View job Code Review
SummaryThis PR adds a well-implemented code treemap visualization (FileGraph) that shows repository structure by lines of code. The implementation is clean, follows existing patterns, and includes proper error handling. However, there are several performance concerns and potential bugs that should be addressed. Critical Issues1. Performance: Synchronous File Reading in getFileGraphLocation: The for (const relativePath of filePaths) {
const fullPath = path.join(repoPath, relativePath)
const lines = await countFileLines(fullPath) // Reads file to count lines
// ...
}Issue: For large repositories with thousands of files, this will:
Recommendation: Implement batching with concurrent reads: const BATCH_SIZE = 50
for (let i = 0; i < filePaths.length; i += BATCH_SIZE) {
const batch = filePaths.slice(i, i + BATCH_SIZE)
await Promise.all(batch.map(async (relativePath) => {
// process file
}))
}Or consider using a worker thread/background process for large scans. 2. Bug: Parent Line Count Updates Don't Work CorrectlyLocation: The code attempts to update parent directory line counts, but the logic is flawed: let parent: FileNode | undefined = root
for (let j = 0; j < parts.length - 1; j++) {
const dirName = parts[j]
parent = parent.children?.find(c => c.name === dirName && c.isDirectory)
if (parent) parent.lines += lines
}Issue: This updates parent nodes during the file loop, but parent directories might not exist yet when processing files in arbitrary order. You then update Recommendation: Build the tree first, then calculate line counts bottom-up in a separate pass, or ensure directories are created before processing their files. 3. Memory Concern: Empty Line FilteringLocation: const content = await fs.promises.readFile(filePath, 'utf-8')
return content.split('\n').filter(line => line.trim().length > 0).lengthIssue: For large files (e.g., minified JS, lockfiles), this loads the entire file into memory, splits it, and filters it. This could cause memory issues. Recommendation: Use a streaming approach or just count newlines without filtering if performance is critical: const content = await fs.promises.readFile(filePath, 'utf-8')
return content.split('\n').lengthOr clarify in comments if blank line filtering is intentional for the visualization. High Priority Issues4. Accessibility: Missing Keyboard NavigationLocation: The treemap only supports mouse clicks for navigation. Users cannot navigate the treemap via keyboard. Recommendation: Add keyboard handlers (Enter/Space to drill down, Escape/Backspace to go up, arrow keys for focus). 5. Empty File HandlingLocation: Files with 0 lines are skipped: if (lines === 0) continueIssue: This is probably correct, but it means truly empty files (valid but 0 lines) won't appear in the tree. This could be confusing if a user is looking for a specific file. Recommendation: Add a comment explaining this is intentional, or set a minimum of 1 line for empty files. 6. Tooltip PositioningLocation: The tooltip uses fixed positioning with a simple offset: style={{ left: mousePos.x + 12, top: mousePos.y + 12 }}Issue: The tooltip can overflow the viewport edges, making it unreadable. Recommendation: Add boundary detection to flip tooltip position when near edges: const shouldFlipX = mousePos.x + 300 > window.innerWidth
const shouldFlipY = mousePos.y + 100 > window.innerHeight
style={{
left: shouldFlipX ? mousePos.x - 312 : mousePos.x + 12,
top: shouldFlipY ? mousePos.y - 112 : mousePos.y + 12
}}Medium Priority Issues7. Hardcoded Legend LimitLocation: return data.languages.slice(0, 8).map((lang) => ({Issue: The legend is limited to 8 languages, but the palette also has 8 colors. If there are more languages, they won't be shown in the legend but will appear in the visualization. Recommendation: Either show all languages (scrollable legend) or add a "Others" category for languages 9+. 8. Label Truncation AlgorithmLocation: function truncateLabel(label: string, width: number): string {
const maxChars = Math.floor(width / 7)
if (label.length <= maxChars) return label
if (maxChars <= 3) return ''
return label.slice(0, maxChars - 2) + '…'
}Issue: The character width estimation (width / 7) is a rough approximation. Different fonts and characters have different widths, so labels might still overflow or be unnecessarily truncated. Recommendation: Use actual SVG text measurement or canvas measureText() for more accurate truncation, or accept the approximation and add a comment explaining it's intentional. 9. Missing Error State UILocation: The component has CSS for Recommendation: Add proper error state rendering to distinguish between "no files" and "scan failed". 10. Inconsistent Language DetectionLocation: The
Recommendation: Review language mappings for accuracy. Consider using a library like Low Priority / Style Issues11. Magic Number: Treemap PaddingLocation: const padding = 2Recommendation: Extract to a constant like 12. Inconsistent Naming: "Code Map" vs "FileGraph"The feature is called "FileGraph" internally but "Code Map" in the UI. Pick one consistent name. 13. Missing JSDoc for Public FunctionsThe 14. Unused VariableLocation: const ext = path.extname(relativePath).slice(1).toLowerCase()This is computed but only used immediately for the binary check. Not a bug, but could be inlined. Positive Observations✅ Good separation of concerns - Layout algorithm is separate from rendering Testing Recommendations
Final RecommendationApprove with changes required for performance issues. The implementation is solid, but the synchronous file reading in large repos will cause significant UX problems. Address issues #1-3 before merging. | |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| 'var(--chart-6)', | ||
| 'var(--chart-7)', | ||
| 'var(--chart-8)', | ||
| ] |
There was a problem hiding this comment.
Missing CSS variables for chart colors
High Severity
The code references CSS variables var(--chart-6), var(--chart-7), and var(--chart-8) in the CHART_PALETTE array and as FALLBACK_LANGUAGE_COLOR, but these variables are not defined in the theme CSS files. Only --chart-1 through --chart-5 are defined in app/styles/globals.css. When these undefined variables are used, they resolve to invalid CSS values, causing visual rendering failures in charts, the FileGraph treemap, and other visualizations that cycle through the color palette.
Additional Locations (2)
2 participants
Note
Adds a new repository "Code Map" treemap visualization and wires it through UI and backend.
file-graphviz panel: interactive treemap (drill-down folders, breadcrumb), language legend, hover tooltips; exposed in Settings panel and Canvas renderergetFileGraphservice to scan repo viagit ls-files, count non-empty lines, detect language by extension, build hierarchicalFileGraphData; added IPC handler, preload API, and Zod schemasFileNode/FileGraphDatatypes, treemap layout helper, and comprehensive CSS for FileGraphfileGraphandfileGraphLoadingto canvasesGitGraph,ContributorChart, andTechTreeChart, plus minor icon tweaksWritten by Cursor Bugbot for commit e6709b7. This will update automatically on new commits. Configure here.