Don't eager load organisation games

[tudddorrr]

No description provided.

[claude]

Code Review for PR #600: Don't eager load organisation games

Summary

This PR removes eager loading from the Organisation.games relationship and makes targeted adjustments to maintain proper data loading where needed. The changes focus on performance optimization by avoiding unnecessary data loading.

✅ Positive Aspects

Performance Improvement:

• Primary Goal Achieved: Removing eager: true from Organisation.games relationship prevents automatic loading of all games whenever an organisation is fetched
• Selective Loading: The changes in user.service.ts:97 use em.populate(user, ['organisation.games']) to explicitly load games only when needed
• Optimized Query: In policy.ts:45, changing from populate: ['organisation'] to populate: ['organisation.id'] loads only the ID field rather than the full organisation entity

Code Quality:

• Consistent API Usage: The change from em.getRepository(Game) to em.repo(Game) in policy.ts:43 uses the more modern MikroORM API
• Proper Population Strategy: Using selective population is a best practice for performance optimization
• Maintained Functionality: The /me endpoint still provides complete organisation data including games where the frontend expects it

⚠️ Concerns & Recommendations

1. Missing Documentation:

• Consider adding a comment in user.service.ts:96-97 explaining why explicit population is needed
• The current comment is helpful but could be more specific about the performance trade-off

2. Potential N+1 Query Risk:

• With eager loading removed, ensure that code accessing organisation.games elsewhere in the codebase doesn't create N+1 query problems
• Recommendation: Run a search for organisation.games usage across the codebase to verify no unintentional lazy loading occurs

3. Test Coverage:

• The test changes appear to be unrelated refactoring (changing currentLevel to someRandomProp)
• Question: Are there performance tests to verify the optimization impact?

🔍 Security & Bug Analysis

Security: ✅ No security concerns identified

• The changes maintain proper access control in canAccessGame method
• Population changes don't expose additional data

Potential Bugs: ⚠️ Low risk

• The toJSON() method in organisation.ts:32 references this.games directly
• With eager loading removed, this could return an uninitialized Collection unless games are explicitly populated
• Risk: If Organisation.toJSON() is called without populating games, it might return unexpected results

🎯 Performance Assessment

Expected Benefits:

• Reduced memory usage when fetching organisations
• Faster queries for organisation-only operations
• Better control over when games data is loaded

Measurement Suggestion:

• Monitor query execution times for organisation fetches before/after deployment
• Watch for any increase in query count due to lazy loading

📋 Recommendations

1. Add explicit population in any code paths that call organisation.toJSON() and expect games data
2. Consider adding a database index on game.organisation_id if not already present (for the populate queries)
3. Run integration tests to ensure the /me endpoint returns complete data as expected
4. Monitor production performance after deployment to validate the optimization

🏁 Overall Assessment

This is a well-targeted performance optimization that follows MikroORM best practices. The changes are minimal and focused, with proper compensation for the removed eager loading where needed.

Verdict: ✅ Approve with minor monitoring recommended

The code quality is good and the performance benefits are clear. The main risk is the potential impact on organisation.toJSON() calls, but this appears to be properly handled in the main usage path (/me endpoint).

---

Generated with Claude Code - focusing on performance, maintainability, and potential edge cases.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.37%. Comparing base (5a1d1aa) to head (3dfce2d).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #600   +/-   ##
========================================
  Coverage    99.37%   99.37%           
========================================
  Files          208      208           
  Lines        14484    14486    +2     
  Branches      1290     1292    +2     
========================================
+ Hits         14393    14395    +2     
  Misses          88       88           
  Partials         3        3           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.