Skip to content

Conversation

pmattmann
Copy link
Member

resolves #7635

@pmattmann pmattmann requested review from carlobeltrame and a team June 24, 2025 21:23
@pmattmann pmattmann changed the title fix n+1 api-requests fix n+1 api-requests on admin/checklist Jun 24, 2025
@BacLuc BacLuc added the deploy! Creates a feature branch deployment for this PR label Jun 26, 2025
@BacLuc BacLuc temporarily deployed to feature-branch June 26, 2025 22:06 — with GitHub Actions Inactive
Copy link

github-actions bot commented Jun 26, 2025

Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

@BacLuc
Copy link
Contributor

BacLuc commented Jun 28, 2025

@CodeRabbit review this pr

Copy link

coderabbitai bot commented Jun 28, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll help you review this PR! Let me gather the necessary information about the changes first.


🏁 Script executed:

#!/bin/bash
# Get PR information including title, description, and files changed
gh pr view 7641 --json title,body,files,additions,deletions,changedFiles

# Get the diff for this PR
gh pr diff 7641

Length of output: 3150


@BacLuc Here's my review of PR #7641:

Overall Assessment: Good Performance Fix

This PR effectively addresses the N+1 API request problem by refactoring the activitiesWithChecklistItem computed property into a more efficient async watcher pattern.

Strengths:

  1. 🚀 Performance Improvement: Eliminates the N+1 API calls that were happening every time the computed property was accessed
  2. ⚡ Efficient Async Handling: Uses Promise.all to run API calls concurrently rather than sequentially
  3. 🎯 Smart Reactivity: Only recalculates when checklistItem changes, not on every access
  4. 📊 Better API Usage: Uses $href() method for URI resolution, which is likely more efficient than the previous approach

Areas for Enhancement:

  1. Error Handling: Consider adding try-catch blocks around the async operations:
async handler(checklistItem) {
  try {
    await this.camp.activities()._meta.load
    // ... rest of the logic
    this.activitiesWithChecklistItem = res
  } catch (error) {
    console.error('Error loading activities with checklist item:', error)
    this.activitiesWithChecklistItem = []
  }
}
  1. Loading State: Consider adding a loading indicator while the async operations are running:
data() {
  return {
    entityProperties: ['checklist', 'text'],
    entityUri: '',
    activitiesWithChecklistItem: [],
    isLoadingActivities: false,
  }
}
  1. Race Condition Protection: If checklistItem changes rapidly, consider debouncing or using a request ID to prevent stale data:
async handler(checklistItem) {
  const requestId = Date.now()
  this.currentRequestId = requestId
  
  // ... async operations ...
  
  if (this.currentRequestId === requestId) {
    this.activitiesWithChecklistItem = res
  }
}

Technical Notes:

  • The refactoring from computed to watcher + data is appropriate for this async scenario
  • The immediate: true option ensures the data loads on component creation
  • The sorting logic remains unchanged, maintaining existing functionality

Conclusion:

This is a solid performance optimization that should significantly reduce API calls. The core implementation is sound, and the suggested enhancements would make it more robust for production use.

Recommendation: ✅ Approve with optional improvements for error handling and loading states.

@BacLuc BacLuc removed the deploy! Creates a feature branch deployment for this PR label Aug 30, 2025
@carlobeltrame carlobeltrame removed request for a team and carlobeltrame September 3, 2025 09:44
@usu usu added the deploy! Creates a feature branch deployment for this PR label Oct 3, 2025
@usu usu temporarily deployed to feature-branch October 3, 2025 19:02 — with GitHub Actions Inactive
@carlobeltrame carlobeltrame added deploy! Creates a feature branch deployment for this PR and removed deploy! Creates a feature branch deployment for this PR labels Oct 3, 2025
@carlobeltrame carlobeltrame force-pushed the feature/admin-checklist-loading-performance branch from 353334d to 5c56a59 Compare October 3, 2025 21:15
@carlobeltrame carlobeltrame force-pushed the feature/admin-checklist-loading-performance branch from 5c56a59 to 1c07342 Compare October 3, 2025 21:37
@carlobeltrame carlobeltrame force-pushed the feature/admin-checklist-loading-performance branch from 1c07342 to 36f69fe Compare October 3, 2025 22:02
@simfeld simfeld force-pushed the feature/admin-checklist-loading-performance branch from 36f69fe to c135a0f Compare October 3, 2025 23:22
Copy link
Member

@manuelmeister manuelmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by @pmattmann

@pmattmann pmattmann added this pull request to the merge queue Oct 4, 2025
Merged via the queue into ecamp:devel with commit 46110b9 Oct 4, 2025
35 checks passed
@pmattmann pmattmann deleted the feature/admin-checklist-loading-performance branch October 4, 2025 06:59
This was referenced Oct 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
before-vue3 deploy! Creates a feature branch deployment for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On camp admin -> checklists, we have n+1 requests
6 participants