-
Notifications
You must be signed in to change notification settings - Fork 7
Project page performance improvements #116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Performance improvements to address ~12s load times for large projects: Git Operations: - Remove synchronous git fetch from page loads - Add lightweight git_dirty_quick_check method without caching - Create separate git_status and git_status_with_fetch methods - Make git status checks async via JavaScript controllers - Increase git_status cache TTL from 1 to 5 minutes - Disable automatic fetch in OptimizedGitStatusService File Scanning: - Exclude common directories from swarm file search (node_modules, vendor, etc) - Skip all directories starting with dot (.) - Limit search depth to 5 directory levels - Add 10-minute caching for find_swarm_files results - Make swarm file count load asynchronously New Endpoints: - /projects/:id/git_dirty_check - Lightweight dirty status check - /projects/:id/swarm_count - Async swarm file count Impact: - Initial page load reduced from ~12s to <1s for large repos - Git operations only run when explicitly needed - File scanning avoids irrelevant directories - Results are cached and reused efficiently Tests: - Added comprehensive tests for all new functionality - All 736 tests passing with 1777 assertions
ccf6577 to
07fed37
Compare
…erations - Remove trailing spaces from all JavaScript files - Replace Shellwords escaping with Open3 array form in sessions_controller.rb - Update test files to use Open3.capture3 instead of system() calls - Improve safety by using Open3's automatic escaping instead of manual Shellwords
07fed37 to
4e9d2d5
Compare
|
/review |
TravisTheTechie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance Review Complete 🚀
Excellent work on achieving a 48x performance improvement (12s → 250ms)! The migration to Open3 also significantly improves security. Here are the critical issues to address:
🔴 Critical Issues
1. Cache Coherency Problem
The different TTLs create inconsistent state:
git_statuscached for 5 minutesgit_dirtycached for 1 minute- A project could show as clean in status but dirty in badge
Fix: Use consistent TTLs or implement hierarchical cache invalidation.
2. Performance Degradation with Large Projects
should_ignore_path? spawns a new git process for EVERY file checked. For projects with 1000+ files, this could spawn thousands of processes.
Fix: Batch git check-ignore operations or cache results.
🟡 Important Issues
3. Swarm Count Badge Stale After Operations
After deleting swarm files via UI, the badge count remains stale until page refresh.
Fix: Add event listener in swarm_count_badge_controller.js for swarm file changes.
4. Missing Cache Invalidation
- Project path updates don't clear git caches
- Branch switching lacks explicit cache clearing
Fix: Add after_update callback for path changes.
5. Request Duplication
No deduplication for concurrent async requests - multiple badge loads trigger duplicate API calls.
Fix: Implement request debouncing/deduplication in Stimulus controllers.
📋 Recommendations
Immediate:
- Fix cache TTL inconsistency
- Add documentation for caching strategy
- Batch git check-ignore operations
Testing Gaps:
- Add tests for cache race conditions
- Test concurrent async request handling
- Add performance benchmarks to prevent regression
The performance gains are impressive and the Open3 migration is excellent for security. Please address the cache coherency and performance scaling issues before merging.
|
|
||
| def git_status | ||
| Rails.cache.fetch("project_#{id}_git_status", expires_in: 1.minute) do | ||
| Rails.cache.fetch("project_#{id}_git_status", expires_in: 5.minutes) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to extract the cache time to a constant and use it consistently.
| } | ||
| rescue StandardError => e | ||
| Rails.logger.warn("Error reading swarm file #{file}: #{e.message}") | ||
| Rails.cache.fetch("project_#{id}_swarm_files", expires_in: 10.minutes) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10 minutes is way too long. I don't think I like this implementation...
- we should skip hidden files, that was added but got clobbered with the git ignore implementation.
- It's still likely the slowest operation we have here. Even on relatively small projects.
- A cache isn't bad, but I think it's more useful to load the dropdown while still indicating it's checking for new entries.
- Maybe we register theses in the DB and make some actively do the search for new yaml files instead of it happening every time the object needs to fulfill the list? That might be the best of both worlds type of thing.
- I thought git operations were slow for projects in Core; but turns out it's actually this that's causing the biggest lags. The git operations are all short circuited because there's no
./.gitfolder in the project root.
| return false unless git? | ||
|
|
||
| # Get relative path from project root | ||
| relative_path = file_path.sub("#{path}/", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need this complexity of checking relative paths, just the fully path should still resolve? I'll check that out.
| message = run_git_command("log -1 --pretty=%s").strip | ||
| author = run_git_command("log -1 --pretty=%an").strip | ||
| date = run_git_command("log -1 --pretty=%ar").strip | ||
| results, _, _ = run_git_command(["log", "-1", "--pretty=%H%n%an%n%ar%n%s"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add a comment on the pretty format here. TODO for me.
| # Fetch from remote to ensure ahead/behind counts are accurate | ||
| git fetch --quiet 2>/dev/null || true | ||
| # Don't fetch by default - it's too expensive for large repos | ||
| # Users can manually sync when needed | ||
| # git fetch --quiet 2>/dev/null || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right. Let me revisit it.
Summary
Claude completely missed the main reason for doing this... this increases the performance of loading pages, such as
/projectsand/projects/:id.dev cd shopifygoes from 12s to 250ms to render the page. I redirected a couple of decisions Claude made, such as usingsystem+Shellwordsinstead of just usingOpen3everywhere. And the use ofgit ignore-checkinstead of attempting to a) understand.gitignoreor maintain a different set of ignored files.This PR improves code quality and security by cleaning up trailing spaces across all JavaScript files and replacing shell command execution with the safer Open3 library for git operations.
Changes
JavaScript Code Cleanup
app/javascript/Git Operations Security Improvement
Shellwords.escape()withOpen3.capture3()array form insessions_controller.rbOpen3.capture3()instead ofsystem()callsFiles Changed
app/controllers/sessions_controller.rb- Removed Shellwords dependencyapp/models/project.rb- Already using Open3 correctlytest/services/git_service_test.rb- Replaced system() with Open3test/models/project_test.rb- Replaced system() with Open3Testing
Migration Notes
None required - these are internal implementation improvements with no API changes.