Skip to content

feat(analysis): add delete analysis functionality#36

Open
karanpraja902 wants to merge 1 commit intomainfrom
feature
Open

feat(analysis): add delete analysis functionality#36
karanpraja902 wants to merge 1 commit intomainfrom
feature

Conversation

@karanpraja902
Copy link
Collaborator

@karanpraja902 karanpraja902 commented Nov 7, 2025

User description

User description

  • Implement backend API endpoint for deleting analyses with proper authorization checks
  • Add frontend UI with delete button in analysis list items
  • Prevent deletion of running analyses and add confirmation dialog

Summary by CodeRabbit

  • New Features
    • Users can now delete analyses with a trash button in the interface, including confirmation prompt and loading feedback.
    • Analysis owners and team members can manage deletions with authorization controls.
    • Added pull request data retrieval capability.

CodeAnt-AI Description

Allow users to delete analyses from the list

What Changed

  • Added a Delete button to each analysis in the repository list; clicking it asks for confirmation, shows a loading spinner while the deletion is processed, and refreshes the list when deletion succeeds
  • Added an API endpoint to delete an analysis by ID; only the analysis owner or members of the owner's team can delete an analysis, and deletion is blocked for analyses that are currently running
  • Added a sandbox API function to fetch PR data by ID (returns 404 if not found)

Impact

✅ Users can delete analyses from the list
✅ Prevents deleting running analyses
✅ Team members can remove analyses owned by their team

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.


PR Type

Enhancement


Description

  • Add backend API endpoint to delete analyses with authorization checks

  • Prevent deletion of running analyses with validation

  • Add frontend delete button with confirmation dialog

  • Implement PR data retrieval sandbox API function


Diagram Walkthrough

flowchart LR
  A["Frontend Delete Button"] -->|"DELETE /api/analysis/:id"| B["deleteAnalysis Controller"]
  B -->|"Check Authorization"| C["Verify Owner or Team Member"]
  C -->|"Validate Status"| D["Prevent Running Analysis Deletion"]
  D -->|"Delete Record"| E["Analysis Removed"]
  E -->|"Refresh List"| A
  F["getPrData Controller"] -->|"Query PR Data"| G["Return PR Document"]
Loading

File Walkthrough

Relevant files
Enhancement
analysis.controller.ts
Add deleteAnalysis controller with auth validation             

apps/api/src/controllers/analysis.controller.ts

  • Add deleteAnalysis controller function with authorization checks
  • Verify user is analysis owner or team member before deletion
  • Block deletion of running analyses with appropriate error message
  • Log deletion events and return success response
+45/-0   
sandbox.controller.ts
Add getPrData sandbox API function                                             

apps/api/src/controllers/sandbox.controller.ts

  • Create new sandbox controller file with getPrData function
  • Query MongoDB pull_request_datas collection by ID
  • Return 404 error if PR data not found
  • Handle cast errors gracefully with try-catch
+30/-0   
analysis.routes.ts
Add DELETE route for analysis deletion                                     

apps/api/src/routes/analysis.routes.ts

  • Import deleteAnalysis controller function
  • Add DELETE route /:id with baseAuth middleware
  • Route placed after status and logs endpoints
+3/-0     
AnalysisContent.tsx
Add frontend delete button with confirmation                         

apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx

  • Import Trash2 icon and Tooltip component for delete UI
  • Add useAuth hook to retrieve authentication token
  • Implement handleDelete function with confirmation dialog and loading
    state
  • Add delete button with hover effect, disabled state for running
    analyses
  • Display loading spinner during deletion and refresh analysis list on
    success
+78/-9   

- Implement backend API endpoint for deleting analyses with proper authorization checks
- Add frontend UI with delete button in analysis list items
- Prevent deletion of running analyses and add confirmation dialog
@codeant-ai
Copy link

codeant-ai bot commented Nov 7, 2025

CodeAnt AI is reviewing your PR.


Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@vercel
Copy link
Contributor

vercel bot commented Nov 7, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
beetle-ai Ready Ready Preview Comment Nov 7, 2025 0:06am

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 7, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

This PR adds a complete delete analysis feature across backend and frontend layers, including a new controller with authorization checks and status validation, a DELETE route endpoint, and corresponding UI components with loading states and confirmation flows.

Changes

Cohort / File(s) Summary
API Controllers
apps/api/src/controllers/analysis.controller.ts, apps/api/src/controllers/sandbox.controller.ts
Added deleteAnalysis function with ownership/team authorization, status validation to prevent deletion of running analyses, and error logging; Added getPrData function to retrieve pull request data by ID with 404 handling.
API Routes
apps/api/src/routes/analysis.routes.ts
Registered new DELETE /:id endpoint wired to deleteAnalysis controller with baseAuth middleware.
Frontend UI
apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx
Implemented delete button with Trash2 icon, per-item deletion loading state tracking, authenticated DELETE request flow with token retrieval, confirmation prompt, error handling, and list refresh on success.

Sequence Diagram

sequenceDiagram
    participant User
    participant Frontend as AnalysisContent
    participant Auth as Auth Service
    participant Backend as deleteAnalysis
    participant DB as MongoDB

    User->>Frontend: Click delete button
    Frontend->>Frontend: Show confirmation prompt
    User->>Frontend: Confirm deletion
    Frontend->>Auth: Retrieve auth token
    Auth-->>Frontend: Token
    Frontend->>Backend: DELETE /api/analysis/{id}<br/>(with token)
    Backend->>Backend: Validate authorization<br/>(owner or team member)
    alt Unauthorized
        Backend-->>Frontend: 403 Forbidden
        Frontend->>User: Show error
    else Status Running
        Backend->>Backend: Check analysis.status
        Backend-->>Frontend: 400 Bad Request
        Frontend->>User: Show error
    else Success
        Backend->>DB: Delete analysis document
        DB-->>Backend: Deletion confirmed
        Backend->>Backend: Log deletion (analysisId, userId)
        Backend-->>Frontend: 200 Success (deleted id)
        Frontend->>Frontend: refreshAnalysisList()
        Frontend->>User: Update UI
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Authorization logic requires verification of ownership and team membership checks
  • Status validation for "running" analyses prevents concurrent deletions
  • Token retrieval and error propagation flow across frontend-backend boundary
  • Multiple async state transitions and edge cases in UI deletion flow

Possibly related PRs

Suggested labels

size:L

Poem

🐰 A dusty bin, a trash icon bright,
Analyses deleted with a click of might,
But running tasks stay safe and sound,
Authorization guards what's been found,
Gone with a swoosh, refreshed in sight! 🗑️

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main feature added across the changeset: delete analysis functionality implemented both backend and frontend.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codeant-ai codeant-ai bot added the size:L This PR changes 100-499 lines, ignoring generated files label Nov 7, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Nitpick comments (1)
apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx (1)

105-106: Consider a more accessible confirmation dialog.

Using window.confirm is not accessible and provides poor UX. Consider using a custom confirmation dialog component that's keyboard-accessible and visually consistent with your design system.

Example implementation using a Dialog component:

// Define state for confirmation dialog
const [deleteConfirmId, setDeleteConfirmId] = useState<string | null>(null);

// In the delete button onClick:
onClick={(e) => {
  e.preventDefault();
  e.stopPropagation();
  setDeleteConfirmId(analysis._id);
}}

// Add Dialog component in JSX
<Dialog open={!!deleteConfirmId} onOpenChange={() => setDeleteConfirmId(null)}>
  <DialogContent>
    <DialogHeader>
      <DialogTitle>Delete Analysis</DialogTitle>
      <DialogDescription>
        Are you sure? This action cannot be undone.
      </DialogDescription>
    </DialogHeader>
    <DialogFooter>
      <Button variant="outline" onClick={() => setDeleteConfirmId(null)}>
        Cancel
      </Button>
      <Button 
        variant="destructive" 
        onClick={() => {
          handleDelete(deleteConfirmId!);
          setDeleteConfirmId(null);
        }}
      >
        Delete
      </Button>
    </DialogFooter>
  </DialogContent>
</Dialog>
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between faf8a5e and 399e85b.

📒 Files selected for processing (4)
  • apps/api/src/controllers/analysis.controller.ts (1 hunks)
  • apps/api/src/controllers/sandbox.controller.ts (1 hunks)
  • apps/api/src/routes/analysis.routes.ts (2 hunks)
  • apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx (5 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/api/src/controllers/analysis.controller.ts (2)
apps/api/src/middlewares/error.ts (1)
  • CustomError (8-14)
apps/api/src/utils/logger.ts (2)
  • logger (155-155)
  • error (115-117)
apps/api/src/controllers/sandbox.controller.ts (1)
apps/api/src/middlewares/error.ts (1)
  • CustomError (8-14)
apps/api/src/routes/analysis.routes.ts (2)
apps/api/src/middlewares/checkAuth.ts (1)
  • baseAuth (60-110)
apps/api/src/controllers/analysis.controller.ts (1)
  • deleteAnalysis (572-615)
apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx (3)
apps/web/lib/_config.ts (1)
  • _config (1-3)
apps/web/app/(root)/analysis/[repoId]/_actions/getAnalysiswithId.ts (1)
  • refreshAnalysisList (33-37)
apps/web/lib/utils.ts (1)
  • cn (15-17)
🔇 Additional comments (3)
apps/api/src/controllers/analysis.controller.ts (1)

598-600: LGTM! Good protection against deleting running analyses.

The status check correctly prevents deletion of running analyses and provides a clear error message to stop it first.

apps/api/src/routes/analysis.routes.ts (1)

10-10: LGTM! Route configuration is correct.

The DELETE route is properly configured with baseAuth middleware and positioned correctly in the route order (after specific routes like /pull_requests, before the catch-all /:github_repositoryId). This follows the same pattern as the stop endpoint.

Also applies to: 44-45

apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx (1)

185-185: LGTM! Good UX for preventing invalid deletions.

The button correctly disables deletion for running analyses and during the delete operation, preventing race conditions and aligning with the backend validation.

Comment on lines +587 to +595
if (analysis.userId !== req.user._id) {
const team = await Team.findOne({ ownerId: analysis.userId });
const isMember = team && Array.isArray((team as any).members)
? (team as any).members.includes(req.user._id)
: false;
if (!isMember) {
return next(new CustomError("Unauthorized to delete this analysis", 403));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Verify type-safe comparison for authorization.

The comparison analysis.userId !== req.user._id may not work correctly if one is a string and the other is an ObjectId (Mongoose document). Consider using .toString() or .equals() for reliable comparison.

Apply this diff to ensure type-safe comparison:

-    if (analysis.userId !== req.user._id) {
+    if (analysis.userId.toString() !== req.user._id.toString()) {
       const team = await Team.findOne({ ownerId: analysis.userId });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (analysis.userId !== req.user._id) {
const team = await Team.findOne({ ownerId: analysis.userId });
const isMember = team && Array.isArray((team as any).members)
? (team as any).members.includes(req.user._id)
: false;
if (!isMember) {
return next(new CustomError("Unauthorized to delete this analysis", 403));
}
}
if (analysis.userId.toString() !== req.user._id.toString()) {
const team = await Team.findOne({ ownerId: analysis.userId });
const isMember = team && Array.isArray((team as any).members)
? (team as any).members.includes(req.user._id)
: false;
if (!isMember) {
return next(new CustomError("Unauthorized to delete this analysis", 403));
}
}
🤖 Prompt for AI Agents
In apps/api/src/controllers/analysis.controller.ts around lines 587-595, the
authorization check compares analysis.userId and req.user._id directly which can
fail when one is a Mongoose ObjectId and the other a string; change the
comparison to a type-safe form (e.g., use analysis.userId.equals(req.user._id)
if analysis.userId is an ObjectId, or compare analysis.userId.toString() ===
req.user._id.toString()) and apply the same approach when checking team members
so membership checks use consistent string or ObjectId equality.

return next(new CustomError("Cannot delete a running analysis. Please stop it first.", 400));
}

await Analysis.findByIdAndDelete(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Consider cleanup of associated resources.

Before deleting the analysis, consider whether any associated resources need cleanup (e.g., sandbox resources, Redis buffers, or uploaded files). The stopAnalysis function shows examples of resource cleanup patterns (lines 528-552).

Run this script to check if analyses have associated sandboxIds that might need cleanup:


🏁 Script executed:

#!/bin/bash
# Check if analyses typically have sandboxIds that might need cleanup
rg -n "sandboxId" -A 3 -B 3 --type=ts -g '!node_modules'

Length of output: 15399


I need to examine the actual delete function and compare it with the cleanup patterns in stopAnalysis.


🏁 Script executed:

#!/bin/bash
# Get the delete function implementation around line 602
sed -n '580,620p' apps/api/src/controllers/analysis.controller.ts

Length of output: 1270


🏁 Script executed:

#!/bin/bash
# Get the stopAnalysis function to see cleanup patterns
sed -n '500,560p' apps/api/src/controllers/analysis.controller.ts

Length of output: 2185


🏁 Script executed:

#!/bin/bash
# Check for any Redis cleanup patterns in the controller
rg -n "deleteAnalysisRedisKey|removeFromRedis|del.*Redis" --type=ts -B2 -A2 apps/api/src/controllers/analysis.controller.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check if there's a cleanupAnalysisResources or similar function
rg -n "cleanup|clean.*[Rr]esource" --type=ts -B2 -A2 apps/api/src/controllers/analysis.controller.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check the analysisStreamStore for Redis key management
rg -n "deleteAnalysisRedisKey\|getAnalysisRedisKey" --type=ts -B1 -A1 apps/api/src/utils/analysisStreamStore.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check sandbox cleanup - does Sandbox.kill actually delete or just stop?
rg -n "export.*kill\|\.kill\(" --type=ts -B2 -A2 apps/api/src/config/sandbox.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Look for any documentation on Sandbox resource lifecycle
rg -n "Sandbox\." --type=ts apps/api/src/config/sandbox.ts | head -20

Length of output: 125


🏁 Script executed:

#!/bin/bash
# Check what happens when an analysis is deleted - any cascading deletes?
rg -n "Analysis\.findByIdAndDelete\|deleteOne\|remove" --type=ts -B3 -A3 apps/api/src/controllers/analysis.controller.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Get more context - what other resources are tracked with Analysis?
rg -n "interface.*Analysis\b|type.*Analysis\b" --type=ts apps/api/src/models/analysis.model.ts | head -10

Length of output: 30


🏁 Script executed:

#!/bin/bash
# Check Redis buffer cleanup specifically
rg -n "redis.*del\|getAnalysisRedisKey\|deleteAnalysisRedisKey" --type=ts apps/api/src/utils/analysisStreamStore.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Look for the exact delete endpoint to understand full context
sed -n '590,630p' apps/api/src/controllers/analysis.controller.ts

Length of output: 837


🏁 Script executed:

#!/bin/bash
# Check if analyses need file cleanup (uploaded files)
rg -n "file|upload|s3\|bucket" --type=ts -i apps/api/src/models/analysis.model.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Broader search for resource cleanup in analysis context
rg -n "cleanup\|clean\|delete.*resource" --type=ts -i -B1 -A1 apps/api/src/services/

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check if there's a transaction or hook for Analysis deletion
rg -n "pre.*delete\|post.*delete\|middleware.*delete" --type=ts -B2 -A2 apps/api/src/models/analysis.model.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Look at the complete Analysis schema to understand all tracked resources
cat apps/api/src/models/analysis.model.ts

Length of output: 1930


🏁 Script executed:

#!/bin/bash
# Check if Sandbox resource has a cleanup/delete method
rg -n "class Sandbox\|export.*Sandbox" --type=ts -B2 -A15 apps/api/src/config/sandbox.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Get context around the findByIdAndDelete call - what's the function name and signature
grep -n "findByIdAndDelete" apps/api/src/controllers/analysis.controller.ts -B15 | head -30

Length of output: 671


🏁 Script executed:

#!/bin/bash
# Check all routes and methods in analysis controller to understand delete context
rg -n "async.*delete\|@.*Delete\|DELETE" --type=ts apps/api/src/controllers/analysis.controller.ts -B2 -A1

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check if Redis cleanup happens anywhere else in the delete flow
rg -n "redis\|cache\|buffer" --type=ts -i apps/api/src/controllers/analysis.controller.ts | grep -i "delete\|clean\|remove"

Length of output: 0


🏁 Script executed:

#!/bin/bash
# See what analysisStreamStore exports for cleanup
grep -n "export" apps/api/src/utils/analysisStreamStore.ts

Length of output: 376


🏁 Script executed:

#!/bin/bash
# Check Sandbox API - what methods are available
rg -n "Sandbox\." --type=ts apps/api/src/services/sandbox/executeAnalysis.ts | grep -E "await|return" | head -15

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check if there's a deleteAnalysis function that handles cleanup
rg -n "deleteAnalysis|delete.*nalysis" --type=ts -B2 -A8 apps/api/src/

Length of output: 4612


🏁 Script executed:

#!/bin/bash
# Check redis configuration/usage to understand buffer cleanup patterns
rg -n "redis\|cache" --type=ts -i apps/api/src/controllers/analysis.controller.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Get the complete delete/remove endpoint handler
rg -n "delete\|remove" --type=ts -i -B10 -A10 apps/api/src/controllers/analysis.controller.ts | grep -A10 -B10 "findByIdAndDelete"

Length of output: 0


🏁 Script executed:

#!/bin/bash
# See if there's a dedicated cleanup handler for analyses
fd -e "ts" -e "js" | xargs grep -l "cleanupAnalysis\|deleteAnalysisResources" 2>/dev/null

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check what Redis keys are created for analyses
rg -n "redis.*set\|getAnalysisRedisKey" --type=ts -B2 -A2 apps/api/src/

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Look at the complete context near line 602
sed -n '595,610p' apps/api/src/controllers/analysis.controller.ts

Length of output: 432


🏁 Script executed:

#!/bin/bash
# Check if there's an event listener or hook that cleans up on deletion
rg -n "removeListener\|off\|on.*delete" --type=ts apps/api/src/controllers/analysis.controller.ts

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Get function name and full implementation of the delete endpoint
rg -n "async.*\(.*\)\s*\{" --type=ts apps/api/src/controllers/analysis.controller.ts | grep -B50 "602:" | tail -60

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Simpler: search for the function that contains line 602
awk 'NR==1,NR==602 {if (/^export|^async|^public/) start=NR} NR==602 {for (i=start; i<=NR; i++) print i": "lines[i]} {lines[NR]=$0}' apps/api/src/controllers/analysis.controller.ts | tail -30

Length of output: 1069


🏁 Script executed:

#!/bin/bash
# Get lines 550-620 to see context of delete and stopAnalysis
sed -n '550,620p' apps/api/src/controllers/analysis.controller.ts

Length of output: 2100


🏁 Script executed:

#!/bin/bash
# Check what the stopAnalysis function does for cleanup
sed -n '520,555p' apps/api/src/controllers/analysis.controller.ts

Length of output: 1286


🏁 Script executed:

#!/bin/bash
# Check what happens with Redis after Analysis deletion
grep -n "Analysis\|redis\|cache" apps/api/src/controllers/analysis.controller.ts | sed -n '595,610p'

Length of output: 0


Add sandbox and Redis cleanup to deleteAnalysis before deletion.

The deleteAnalysis function (line 602) directly deletes the analysis without cleaning up associated resources. In comparison, stopAnalysis (lines 526-530) demonstrates the proper cleanup pattern by calling Sandbox.kill() for the sandboxId and appendToRedisBuffer() for Redis buffers.

Since running analyses are already blocked from deletion (line 598-599), the cleanup is needed for paused/completed analyses that may still have active sandbox resources or Redis buffers.

Add cleanup steps before line 602:

  • If analysis.sandboxId exists, call Sandbox.kill(analysis.sandboxId) to release the E2B sandbox
  • Call finalizeAnalysisAndPersist() with status "deleted" (or similar) to finalize Redis buffers, matching the cleanup patterns in stopAnalysis
🤖 Prompt for AI Agents
In apps/api/src/controllers/analysis.controller.ts around line 602,
deleteAnalysis currently calls await Analysis.findByIdAndDelete(id) without
cleaning up associated resources; before the deletion add the cleanup steps: if
analysis.sandboxId exists, await Sandbox.kill(analysis.sandboxId) to release the
E2B sandbox, then await finalizeAnalysisAndPersist(analysis, "deleted") (or the
existing finalize function used in stopAnalysis) to flush/append Redis buffers
and finalize state, and only after both awaits complete call findByIdAndDelete;
ensure proper error handling/awaits and preserve the same cleanup ordering used
in stopAnalysis.

Comment on lines +5 to +30
export const getPrData = async (req: Request, res: Response, next: NextFunction) => {
try {
const { id } = req.params as { id: string };
console.log("🔄 Getting PR data: ", id);

let data = null as any;
try {
data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
// console.log("🔄 Doc: ", doc);
} catch (_) {
// ignore cast errors
}

if (!data) {
return next(new CustomError("PR data not found", 404));
}

return res.status(200).json({
success: true,
data,
});
} catch (error: any) {
console.error("Error fetching PR data:", error);
next(new CustomError(error.message || "Failed to fetch PR data", 500));
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Add authorization check for PR data access.

This endpoint has no authorization check—any authenticated user can access any PR data by ID. This is a security vulnerability if PR data contains sensitive information.

Consider adding authorization similar to other controllers:

 export const getPrData = async (req: Request, res: Response, next: NextFunction) => {
   try {
     const { id } = req.params as { id: string };
-    console.log("🔄 Getting PR data: ", id);
+    logger.debug("Getting PR data", { id, userId: req.user?._id });

     let data = null as any;
     try {
       data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
-      // console.log("🔄 Doc: ", doc);
     } catch (_) {
       // ignore cast errors
     }

     if (!data) {
       return next(new CustomError("PR data not found", 404));
     }

+    // Authorization: verify user owns this PR data or has access
+    if (data.userId && data.userId.toString() !== req.user._id.toString()) {
+      return next(new CustomError("Unauthorized to access this PR data", 403));
+    }

     return res.status(200).json({
       success: true,
       data,
     });
   } catch (error: any) {
-    console.error("Error fetching PR data:", error);
+    logger.error("Error fetching PR data", { error: error?.message || error, id: req.params.id });
     next(new CustomError(error.message || "Failed to fetch PR data", 500));
   }
 }

Committable suggestion skipped: line range outside the PR's diff.

export const getPrData = async (req: Request, res: Response, next: NextFunction) => {
try {
const { id } = req.params as { id: string };
console.log("🔄 Getting PR data: ", id);
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Use logger instead of console for consistency.

The codebase uses a structured logger (see logger.info, logger.debug, etc. throughout the analysis controller). Using console.log and console.error here is inconsistent and loses structured logging benefits.

As per coding guidelines (evident from the consistent use of logger in apps/api/src/controllers/analysis.controller.ts).

Also applies to: 27-27

🤖 Prompt for AI Agents
In apps/api/src/controllers/sandbox.controller.ts around lines 8 and 27, replace
the console.log/console.error calls with the project's structured logger (e.g.,
logger.info or logger.error) to maintain consistency with other controllers;
update the messages to use the logger methods and pass the id/error as
structured metadata rather than raw console output so logs remain uniform and
searchable.

Comment on lines +10 to +16
let data = null as any;
try {
data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
// console.log("🔄 Doc: ", doc);
} catch (_) {
// ignore cast errors
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Empty catch block silently hides errors.

The empty catch block (catch (_)) silently ignores cast errors, which could mask bugs like invalid ID formats. Consider logging the error or using a more specific error type.

Apply this diff:

     let data = null as any;
     try {
       data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
-    } catch (_) {
-      // ignore cast errors
+    } catch (err) {
+      logger.warn("Invalid PR data ID format", { id, error: err });
+      return next(new CustomError("Invalid PR data ID", 400));
     }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In apps/api/src/controllers/sandbox.controller.ts around lines 10 to 16, the
catch block currently swallows all errors; instead validate the id up-front with
mongoose.Types.ObjectId.isValid(id) before querying, and replace the empty catch
with catching the error as a named variable, logging the error (using the app
logger or console.error) and only suppressing known cast/validation errors
(e.g., check err.name === 'CastError' or err instanceof
mongoose.Error.CastError); for any other unexpected error rethrow or return an
error response so it isn't silently hidden.

Comment on lines +104 to +132
const handleDelete = async (id: string) => {
const confirmed = typeof window !== "undefined" ? window.confirm("Delete this analysis? This cannot be undone.") : false;
if (!confirmed) return;

try {
setDeletingIds((prev) => ({ ...prev, [id]: true }));
const token = await getToken();
if (!token || !_config.API_BASE_URL) {
throw new Error("Missing auth token or API base URL");
}
const res = await fetch(`${_config.API_BASE_URL}/api/analysis/${id}`, {
method: "DELETE",
headers: {
Authorization: `Bearer ${token}`,
},
});
if (!res.ok) {
const msg = `Failed to delete analysis (${res.status})`;
throw new Error(msg);
}
// Refresh list; redirect useEffect will handle selection
await refreshAnalysisList();
} catch (err) {
console.error("Delete analysis failed", err);
// Optional: surface error to user via toast (not implemented here)
} finally {
setDeletingIds((prev) => ({ ...prev, [id]: false }));
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add user feedback for delete errors.

The error handling only logs to console (line 127) without providing user feedback. Users won't know if the deletion failed.

Consider adding a toast notification or alert to inform users of errors:

     } catch (err) {
       console.error("Delete analysis failed", err);
-      // Optional: surface error to user via toast (not implemented here)
+      // TODO: Add toast notification library and show error
+      alert("Failed to delete analysis. Please try again.");
     } finally {

Alternatively, consider integrating a proper toast notification library like sonner or react-hot-toast for better UX.

Would you like me to help implement a toast notification system for error feedback?

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const handleDelete = async (id: string) => {
const confirmed = typeof window !== "undefined" ? window.confirm("Delete this analysis? This cannot be undone.") : false;
if (!confirmed) return;
try {
setDeletingIds((prev) => ({ ...prev, [id]: true }));
const token = await getToken();
if (!token || !_config.API_BASE_URL) {
throw new Error("Missing auth token or API base URL");
}
const res = await fetch(`${_config.API_BASE_URL}/api/analysis/${id}`, {
method: "DELETE",
headers: {
Authorization: `Bearer ${token}`,
},
});
if (!res.ok) {
const msg = `Failed to delete analysis (${res.status})`;
throw new Error(msg);
}
// Refresh list; redirect useEffect will handle selection
await refreshAnalysisList();
} catch (err) {
console.error("Delete analysis failed", err);
// Optional: surface error to user via toast (not implemented here)
} finally {
setDeletingIds((prev) => ({ ...prev, [id]: false }));
}
};
const handleDelete = async (id: string) => {
const confirmed = typeof window !== "undefined" ? window.confirm("Delete this analysis? This cannot be undone.") : false;
if (!confirmed) return;
try {
setDeletingIds((prev) => ({ ...prev, [id]: true }));
const token = await getToken();
if (!token || !_config.API_BASE_URL) {
throw new Error("Missing auth token or API base URL");
}
const res = await fetch(`${_config.API_BASE_URL}/api/analysis/${id}`, {
method: "DELETE",
headers: {
Authorization: `Bearer ${token}`,
},
});
if (!res.ok) {
const msg = `Failed to delete analysis (${res.status})`;
throw new Error(msg);
}
// Refresh list; redirect useEffect will handle selection
await refreshAnalysisList();
} catch (err) {
console.error("Delete analysis failed", err);
// TODO: Add toast notification library and show error
alert("Failed to delete analysis. Please try again.");
} finally {
setDeletingIds((prev) => ({ ...prev, [id]: false }));
}
};
🤖 Prompt for AI Agents
In apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx around
lines 104 to 132, the catch block only logs errors to the console so users get
no feedback when deletion fails; add a user-facing toast/alert in the catch to
surface the failure (e.g., import and call your project's toast library like
sonner or react-hot-toast and invoke toast.error with a clear message that
includes err?.message or a fallback string), ensure the toast import is added at
the top of the file, and keep the existing console.error for diagnostics.

Comment on lines +178 to +200
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="ghost"
size="icon"
className="text-muted-foreground"
aria-label="Delete analysis"
disabled={analysis.status === "running" || !!deletingIds[analysis._id]}
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
handleDelete(analysis._id);
}}
>
{deletingIds[analysis._id] ? (
<Loader2 className="h-4 w-4 animate-spin" />
) : (
<Trash2 className="h-4 w-4" />
)}
</Button>
</TooltipTrigger>

</Tooltip>
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing TooltipContent for the delete button.

The Tooltip component wraps the delete button but has no <TooltipContent> child, so no tooltip will be shown. Users won't see an explanation of what the button does or why it's disabled.

Apply this diff to add tooltip content:

                 <Tooltip>
                   <TooltipTrigger asChild>
                     <Button
                       variant="ghost"
                       size="icon"
                       className="text-muted-foreground"
                       aria-label="Delete analysis"
                       disabled={analysis.status === "running" || !!deletingIds[analysis._id]}
                       onClick={(e) => {
                         e.preventDefault();
                         e.stopPropagation();
                         handleDelete(analysis._id);
                       }}
                     >
                       {deletingIds[analysis._id] ? (
                         <Loader2 className="h-4 w-4 animate-spin" />
                       ) : (
                         <Trash2 className="h-4 w-4" />
                       )}
                     </Button>
                   </TooltipTrigger>
+                  <TooltipContent>
+                    {analysis.status === "running" 
+                      ? "Cannot delete running analysis" 
+                      : "Delete analysis"}
+                  </TooltipContent>
                 </Tooltip>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="ghost"
size="icon"
className="text-muted-foreground"
aria-label="Delete analysis"
disabled={analysis.status === "running" || !!deletingIds[analysis._id]}
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
handleDelete(analysis._id);
}}
>
{deletingIds[analysis._id] ? (
<Loader2 className="h-4 w-4 animate-spin" />
) : (
<Trash2 className="h-4 w-4" />
)}
</Button>
</TooltipTrigger>
</Tooltip>
<Tooltip>
<TooltipTrigger asChild>
<Button
variant="ghost"
size="icon"
className="text-muted-foreground"
aria-label="Delete analysis"
disabled={analysis.status === "running" || !!deletingIds[analysis._id]}
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
handleDelete(analysis._id);
}}
>
{deletingIds[analysis._id] ? (
<Loader2 className="h-4 w-4 animate-spin" />
) : (
<Trash2 className="h-4 w-4" />
)}
</Button>
</TooltipTrigger>
<TooltipContent>
{analysis.status === "running"
? "Cannot delete running analysis"
: "Delete analysis"}
</TooltipContent>
</Tooltip>
🤖 Prompt for AI Agents
In apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx around
lines 178 to 200, the Tooltip wrapping the delete Button lacks TooltipContent so
nothing is shown; add a TooltipContent element (with a TooltipArrow if used
elsewhere) as a sibling to TooltipTrigger containing the label such as "Delete
analysis" and optionally a conditional message when the button is disabled
(e.g., "Cannot delete while running") so users can see the action and reason for
disabled state.

)}
</Button>
</TooltipTrigger>

Copy link

Choose a reason for hiding this comment

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

Suggestion: Add a TooltipContent element inside the Tooltip so the tooltip actually shows a label (and uses the imported component). [enhancement]

Severity Level: Minor ⚠️

Suggested change
<TooltipContent>
Delete analysis
</TooltipContent>
Why it matters? ⭐

The current Tooltip is missing a TooltipContent so the tooltip has no visible label. The suggested change simply uses the already-imported TooltipContent to render "Delete analysis" which is a small UI/UX fix — not a functional bug or security issue, but it meaningfully improves affordance. The improved code aligns with existing imports and component patterns in the file.

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx
**Line:** 199:199
**Comment:**
	*Enhancement: Add a TooltipContent element inside the Tooltip so the tooltip actually shows a label (and uses the imported component).

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.

@codeant-ai
Copy link

codeant-ai bot commented Nov 7, 2025

CodeAnt AI finished reviewing your PR.

@shivang-16 shivang-16 closed this Nov 7, 2025
@shivang-16 shivang-16 reopened this Nov 7, 2025
@shivang-16 shivang-16 closed this Nov 7, 2025
@shivang-16 shivang-16 reopened this Nov 7, 2025
@shivang-16 shivang-16 closed this Nov 7, 2025
@shivang-16 shivang-16 reopened this Nov 7, 2025
Repository owner deleted a comment from beetle-ai bot Nov 7, 2025
@shivang-16 shivang-16 closed this Nov 7, 2025
@shivang-16 shivang-16 reopened this Nov 7, 2025
@shivang-16 shivang-16 closed this Nov 7, 2025
@shivang-16 shivang-16 reopened this Nov 7, 2025
Repository owner deleted a comment from beetle-ai bot Nov 7, 2025
Repository owner deleted a comment from beetle-ai bot Nov 7, 2025
Repository owner deleted a comment from beetle-ai bot Nov 7, 2025
Repository owner deleted a comment from beetle-ai bot Nov 7, 2025
Repository owner deleted a comment from beetle-ai bot Nov 7, 2025
Repository owner deleted a comment from beetle-ai bot Nov 7, 2025
@shivang-16 shivang-16 closed this Nov 7, 2025
@shivang-16 shivang-16 reopened this Nov 7, 2025
@beetle-ai
Copy link
Contributor

beetle-ai bot commented Nov 7, 2025

Summary by Beetle

This pull request introduces a new feature allowing users to delete an analysis. This is a full-stack implementation that includes a new backend API endpoint for handling the deletion logic and corresponding UI changes on the frontend to make the feature accessible to users.
The core functionality includes authorization checks to ensure only permitted users can delete an analysis and a business rule that prevents the deletion of analyses that are currently running. The frontend has been updated to include a delete button, a confirmation dialog, and loading states for a smooth user experience.
A seemingly unrelated endpoint for fetching raw pull request data has also been added in sandbox.controller.ts.
File Changes Summary (Consolidated across all commits):

File Status Changes Description
apps/api/src/controllers/analysis.controller.ts Modified +45/-0 Adds the deleteAnalysis controller function. This function contains the core backend logic, including finding the analysis, checking user authorization (owner or team member), and preventing the deletion of 'running' analyses before removing it from the database.
apps/api/src/controllers/sandbox.controller.ts Added +30/-0 Adds a new, seemingly unrelated controller with a getPrData function to fetch specific pull request data directly from a MongoDB collection.
apps/api/src/routes/analysis.routes.ts Modified +3/-0 Registers the new DELETE /api/analysis/:id route, linking it to the deleteAnalysis controller and protecting it with authentication middleware.
apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx Modified +78/-9 Implements the frontend UI for the delete feature. This includes adding a delete icon on hover, a confirmation dialog, handling the API call with auth tokens, and managing loading/disabled states for the button.
Total Changes: 4 files changed, +156 additions, -9 deletions
Walkthrough:
sequenceDiagram
participant User
participant Frontend (AnalysisContent.tsx)
participant API Gateway
participant Backend (analysis.controller.ts)
participant Database (MongoDB)
User->>Frontend: Hovers over analysis item & clicks delete icon
Frontend->>User: Shows confirmation dialog
User->>Frontend: Confirms deletion
Frontend->>API Gateway: Sends DELETE /api/analysis/:id request with Auth Token
API Gateway->>Backend: Forwards request to deleteAnalysis()
Backend->>Database: Find analysis by ID
Database-->>Backend: Returns analysis document
alt Authorization & Validation
Backend->>Backend: Check if user is owner or team member
Backend->>Backend: Check if analysis.status is "running"
else Unauthorized or Invalid
Backend-->>Frontend: Returns 403/400 Error Response
end
Backend->>Database: Executes findByIdAndDelete(id)
Database-->>Backend: Confirms deletion
Backend-->>Frontend: Returns 200 OK { success: true }
Frontend->>Frontend: Refreshes the analysis list in the UI
Loading

Key Changes:

  • Full-Stack Delete Functionality: A complete end-to-end feature for deleting analyses has been implemented, from the UI button to the database record removal.
  • Authorization Logic: The backend endpoint includes a crucial security check to ensure that only the analysis owner or a member of their team can perform the deletion.
  • Status-Based Business Rule: The system now prevents the deletion of any analysis that is currently in a "running" state, which protects ongoing processes.
  • Improved User Experience: The frontend provides clear user feedback with a confirmation dialog before deleting, a loading indicator during the process, and a non-intrusive delete button that only appears on hover.
    Impact Assessment:
  • Security: Positive. The new DELETE endpoint is protected by authentication middleware and has specific authorization logic to prevent unauthorized data modification.
  • Performance: Negligible. The delete operation is a standard indexed database query and should not introduce any performance bottlenecks.
  • Maintainability: Good. The new logic is well-encapsulated within the analysis.controller.ts and the corresponding frontend component, following the existing structure of the application.
  • Testing: The new API endpoint should be covered by integration tests, specifically for the authorization logic, the "running" status check, and successful deletion cases. Frontend tests should be added to verify the UI behavior of the delete button and confirmation flow.

Comment on lines +589 to +591
const isMember = team && Array.isArray((team as any).members)
? (team as any).members.includes(req.user._id)
: false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsafe type casting with (team as any).members bypasses TypeScript's type safety. If the Team model's structure changes or if members is undefined/null, this could cause runtime errors. The Array.isArray() check helps but doesn't prevent accessing undefined properties.

Confidence: 4/5

Suggested Fix Consider properly typing the team object or adding more defensive checks: ```typescript const isMember = team?.members && Array.isArray(team.members) ? team.members.some((memberId: any) => memberId.toString() === req.user._id.toString()) : false; ``` Use optional chaining (`?.`) to safely access the members property and avoid potential runtime errors.

📍 This suggestion applies to lines 589-591

@shivang-16 shivang-16 closed this Nov 13, 2025
@shivang-16 shivang-16 reopened this Nov 13, 2025
@beetle-ai
Copy link
Contributor

beetle-ai bot commented Nov 13, 2025

🪲 Beetle AI is reviewing this PR — Let’s see what you’ve done!

Under Review

Commits (1)
  • 399e85b — feat(analysis): add delete analysis functionality (karanpraja902)
Files Changed (4)
  • apps/api/src/controllers/analysis.controller.ts
  • apps/api/src/controllers/sandbox.controller.ts
  • apps/api/src/routes/analysis.routes.ts
  • apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx

Step aside — I’m tearing through this PR 😈 -- You keep on building


                      ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⡴⠶⠶⠦⠦⣤⣀⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⡤⣞⠁⠀⠀⡄⠀⠀⠀⠀⠐⢣⢄⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⡖⠀⣁⠀⠠⡮⠁⠀⠀⠀⠀⠀⠀⠈⠐⣀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⣏⠐⣣⠦⠐⣧⠆⢀⡰⠃⠰⠆⠀⠀⠀⠀⠸⠆⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⡟⡤⠃⡄⠛⠛⢓⡛⢓⣾⡇⣰⡞⠃⢠⢰⢀⠙⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⡥⡧⣯⣿⠡⢾⡯⢧⡠⠟⠋⢁⡠⢌⣏⣺⢸⣰⠁⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⣀⣀⣁⠇⠇⣇⠀⠀⠁⠀⠀⠀⢀⣠⣄⠈⢸⡿⠸⠋⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⠀⠀⠀⠀⠀⠀⣤⠶⠋⠉⠀⠀⢧⠀⠀⠀⠃⠀⠀⠀⠀⠀⠀⠛⠃⣴⠋⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⠀⠀⠀⢀⠰⠉⠀⠀⠀⠀⠀⡀⠀⡶⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢈⡇⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⠀⠀⠰⠊⠁⠀⠀⠀⠀⠀⠀⠙⡄⠀⠓⣤⡆⣦⡀⠀⠀⠐⣢⡌⠉⠁⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⠀⢀⡘⠃⠀⠀⢀⠶⠀⠀⠀⠀⠀⣏⠀⠀⠙⣣⣏⡹⠦⠔⠚⠁⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⢠⠎⠁⠀⠀⠀⣸⠀⠀⠀⠀⠀⣤⠿⠀⢀⡴⠈⠉⢁⡆⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠀⡖⠀⠀⠀⠀⠰⣤⢀⠀⣤⣄⣀⠶⠀⣠⠎⠓⠋⠉⠉⢛⠳⠤⢄⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⠘⠃⠀⠀⠀⠀⢀⣴⠏⠀⠀⣠⣼⠗⠋⠁⢀⡄⣀⠀⡸⣾⣷⡀⠀⠈⢣⠄⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⢘⡇⠀⠀⠀⠀⠀⢘⠁⠀⠛⠀⠀⠀⠀⣰⠋⠀⣠⠬⠉⠉⠀⡉⠛⠢⣼⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⢸⡇⠀⠀⠠⠞⢋⢹⠀⠀⠀⠀⠀⠀⣬⠁⠀⣼⠓⠲⠤⣄⡬⠁⠀⠀⠀⠈⠙⠢⣄⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀
                      ⢸⣅⣀⡴⢂⡴⠊⠉⣤⣀⣀⠤⠖⠋⠀⠀⡼⠁⠀⠀⢀⡦⠀⠀⠀⠀⠀⠀⠀⠀⠀⠉⠒⢦⣀⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢀⣄⡄⠀⠀⠀⠀⠀⠀⠀⠠⣄⠀⠀⠀⠀⠀
                      ⠰⡏⠁⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⣖⠟⠀⠀⠀⢠⠆⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⠉⠲⢤⣀⠀⠀⠀⠀⠀⠀⠀⠘⠯⠁⠀⢀⣠⣀⣀⠤⣀⡀⠽⠂⠀⠀⠀⠀
                      ⠀⠹⢆⠀⠀⠀⠀⠀⠀⠀⠀⠀⣠⢛⡟⠀⠀⢀⣰⠏⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⠙⠲⠄⠀⠀⢀⡸⠁⢈⣷⡶⢋⡙⣦⣀⣀⠈⠙⠦⡀⠀⠀⠀⠀
                      ⠀⠀⠘⢢⣀⠀⠀⠀⠀⠀⢀⡶⠀⣍⠀⠠⡶⠛⠇⡀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢓⣀⡴⠞⢂⣀⡾⠉⢳⠊⠀⠈⣂⠉⠛⠢⣄⢻⡀⠀⠀⠀
                      ⠀⠀⠀⠀⠈⢹⠐⠒⠚⠛⠉⠙⠛⠀⠋⠋⠉⠓⠾⠟⠓⢲⣄⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⢩⡀⠀⢈⡟⢡⠝⢻⠃⠀⠀⣸⠀⠀⠀⠈⢈⡇⠀⠀⠀
                      ⢀⣀⣠⢴⢒⣸⣄⣀⣀⣠⣴⣀⠀⠀⠀⠀⠀⡀⡀⢔⣀⡠⢇⣀⣰⡦⠤⠄⡠⠄⠀⢀⠀⠀⠀⠀⠀⠀⠀⠀⠀⢀⡀⠙⠤⠴⢾⡾⠲⠸⣤⣀⣰⡂⣀⠀⡀⣠⠮⢤⠀⠀⠀
                      ⠀⠀⠘⠃⠀⠀⠀⠀⠀⠀⠀⠀⠉⠉⠉⠁⠉⠁⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠁⠀⠀⠀⠀⠀⠀⠀⠠⠄⠠⠄⠨⡷⠼⢇⣀⡸⠏⠀⣠⣀⠾⢀⠀⠹⣭⡀⠉⠷⣄⣀⠳⠄⠆
                      ⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠀⠈⠀⠀⠐⠂⢸⠈⠈⠀⠀⠀⠉⠀⠀⠈⠉⠀⠀⠀


Links: Beetle · X · LinkedIn

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Sensitive information exposure

Description: Directly logs request-derived data and raw errors to console and queries MongoDB by a
client-supplied ObjectId without authentication or authorization, enabling potential
sensitive information exposure through logs and unauthenticated data enumeration if the
route is publicly reachable.
sandbox.controller.ts [8-28]

Referred Code
  console.log("🔄 Getting PR data: ", id);

  let data = null as any;
  try {
    data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
    // console.log("🔄 Doc: ", doc);
  } catch (_) {
    // ignore cast errors
  }

  if (!data) {
    return next(new CustomError("PR data not found", 404));
  }

  return res.status(200).json({
    success: true,
    data,
  });
} catch (error: any) {
  console.error("Error fetching PR data:", error);
  next(new CustomError(error.message || "Failed to fetch PR data", 500));
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Console logging: The controller logs internal details to console (including potential ObjectId parsing
issues) which may leak information in user-facing environments instead of using secure
internal logging.

Referred Code
console.log("🔄 Getting PR data: ", id);

let data = null as any;
try {
  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
  // console.log("🔄 Doc: ", doc);
} catch (_) {

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit log: The delete action logs a generic info message but does not appear to create a structured
audit trail entry including user, timestamp, action, target, and outcome.

Referred Code
logger.info("Analysis deleted", { analysisId: id, userId: req.user._id });

res.json({
  success: true,
  message: "Analysis deleted successfully",
  data: { id }
});

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Generic client error: The frontend delete handler throws and logs a generic error without surfacing actionable
feedback to the user or parsing server error details (e.g., 400 running, 403
unauthorized).

Referred Code
  if (!res.ok) {
    const msg = `Failed to delete analysis (${res.status})`;
    throw new Error(msg);
  }
  // Refresh list; redirect useEffect will handle selection
  await refreshAnalysisList();
} catch (err) {
  console.error("Delete analysis failed", err);
  // Optional: surface error to user via toast (not implemented here)
} finally {
  setDeletingIds((prev) => ({ ...prev, [id]: false }));
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Log content review: Logs include userId and error messages; while likely acceptable, verify that no sensitive
data from errors is logged and that logs are structured consistently across controllers.

Referred Code
logger.error("Error stopping analysis", { analysisId: req.params.id, error: error?.message || error });
next(new CustomError(error.message || "Failed to stop analysis", 500));

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
ID validation: The endpoint attempts to construct an ObjectId from user input without explicit
validation, relying on a try/catch and ignoring errors, which may obscure invalid
requests.

Referred Code
try {
  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
  // console.log("🔄 Doc: ", doc);
} catch (_) {
  // ignore cast errors
}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect ObjectId comparison logic

Fix the authorization logic by using the .equals() method to correctly compare
Mongoose ObjectId values instead of using the !== operator.

apps/api/src/controllers/analysis.controller.ts [586-595]

 // Authorization: user must be owner or part of the team owned by analysis.userId
-if (analysis.userId !== req.user._id) {
+if (!analysis.userId.equals(req.user._id)) {
   const team = await Team.findOne({ ownerId: analysis.userId });
   const isMember = team && Array.isArray((team as any).members)
-    ? (team as any).members.includes(req.user._id)
+    ? (team as any).members.some((memberId: any) => memberId.equals(req.user._id))
     : false;
   if (!isMember) {
     return next(new CustomError("Unauthorized to delete this analysis", 403));
   }
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: This suggestion identifies a critical bug in the authorization logic where Mongoose ObjectIds are compared by reference (!==) instead of by value, which would cause the authorization to fail incorrectly. Applying this fix is essential for the feature to work as intended.

High
Prevent multiple delete requests on click

Prevent a race condition in handleDelete by adding a check at the beginning of
the function to see if a deletion for the given id is already in progress.

apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx [104-132]

 const handleDelete = async (id: string) => {
+  if (deletingIds[id]) return; // Prevent multiple clicks
+
   const confirmed = typeof window !== "undefined" ? window.confirm("Delete this analysis? This cannot be undone.") : false;
   if (!confirmed) return;
 
   try {
     setDeletingIds((prev) => ({ ...prev, [id]: true }));
     const token = await getToken();
     if (!token || !_config.API_BASE_URL) {
       throw new Error("Missing auth token or API base URL");
     }
     const res = await fetch(`${_config.API_BASE_URL}/api/analysis/${id}`, {
       method: "DELETE",
       headers: {
         Authorization: `Bearer ${token}`,
       },
     });
     if (!res.ok) {
-      const msg = `Failed to delete analysis (${res.status})`;
+      const errorData = await res.json().catch(() => null);
+      const msg = errorData?.message || `Failed to delete analysis (${res.status})`;
       throw new Error(msg);
     }
     // Refresh list; redirect useEffect will handle selection
     await refreshAnalysisList();
   } catch (err) {
     console.error("Delete analysis failed", err);
     // Optional: surface error to user via toast (not implemented here)
   } finally {
     setDeletingIds((prev) => ({ ...prev, [id]: false }));
   }
 };
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies a potential race condition, although the existing code already mitigates it by disabling the button. The proposed check adds an extra layer of protection, making the function more robust against rapid clicks.

Low
High-level
PR includes an unrelated, incomplete feature

The PR adds a new getPrData controller, but this feature is unrelated to the
main goal of deleting analyses. Since it is not connected to any API route, it
constitutes dead code and should be removed from this PR.

Examples:

apps/api/src/controllers/sandbox.controller.ts [1-30]
import { NextFunction, Request, Response } from "express";
import { CustomError } from "../middlewares/error.js";
import mongoose from "mongoose";

export const getPrData = async (req: Request, res: Response, next: NextFunction) => {
  try {
    const { id } = req.params as { id: string };
    console.log("🔄 Getting PR data: ", id);

    let data = null as any;

 ... (clipped 20 lines)

Solution Walkthrough:

Before:

// File: 'apps/api/src/controllers/sandbox.controller.ts' (new file)
import { NextFunction, Request, Response } from "express";
import { CustomError } from "../middlewares/error.js";
import mongoose from "mongoose";

export const getPrData = async (req: Request, res: Response, next: NextFunction) => {
  try {
    const { id } = req.params;
    // ... logic to fetch data from 'pull_request_datas' collection
    let data = await mongoose.connection.db?.collection('pull_request_datas').findOne(...);
    if (!data) {
      return next(new CustomError("PR data not found", 404));
    }
    return res.status(200).json({ success: true, data });
  } catch (error: any) {
    next(new CustomError(error.message, 500));
  }
}
// Note: No route is added to expose this controller function.

After:

// File: 'apps/api/src/controllers/sandbox.controller.ts'
// This file is removed entirely from the pull request.
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the new getPrData controller is unrelated to the PR's main feature and is dead code because it's not exposed via any route, making this a significant code quality and PR scope issue.

Medium
General
Improve database query and error handling

Improve the database query by validating the id format before use and by adding
proper error handling instead of silently ignoring exceptions in the try-catch
block.

apps/api/src/controllers/sandbox.controller.ts [10-16]

 let data = null as any;
 try {
+  if (!mongoose.Types.ObjectId.isValid(id)) {
+    return next(new CustomError("Invalid ID format", 400));
+  }
   data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
-  // console.log("🔄 Doc: ", doc);
-} catch (_) {
-  // ignore cast errors
+} catch (error) {
+  console.error("Error fetching from pull_request_datas:", error);
+  // Decide if you want to bubble up the error or continue
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that silently swallowing database errors is a poor practice. Adding input validation for the ID and handling errors properly improves the endpoint's robustness and makes it easier to debug.

Medium
  • More

@shivang-16 shivang-16 closed this Nov 13, 2025
@shivang-16 shivang-16 reopened this Nov 13, 2025
@beetle-ai
Copy link
Contributor

beetle-ai bot commented Nov 13, 2025

🪲 Daily PR Analysis Limit Reached

You've hit the daily limits of PR analysis. Consider updating the plan: https://beetleai.dev/dashboard


Links: Beetle · X · LinkedIn

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Insecure logging

Description: The API logs sensitive request context directly to stdout using console.log/console.error
(lines 8 and 27), which can leak potentially sensitive identifiers or error details and
bypass centralized logging controls.
sandbox.controller.ts [8-28]

Referred Code
  console.log("🔄 Getting PR data: ", id);

  let data = null as any;
  try {
    data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
    // console.log("🔄 Doc: ", doc);
  } catch (_) {
    // ignore cast errors
  }

  if (!data) {
    return next(new CustomError("PR data not found", 404));
  }

  return res.status(200).json({
    success: true,
    data,
  });
} catch (error: any) {
  console.error("Error fetching PR data:", error);
  next(new CustomError(error.message || "Failed to fetch PR data", 500));
Faulty auth check

Description: Authorization check compares MongoDB ObjectIds with strict inequality (analysis.userId !==
req.user._id) which may fail due to type/reference differences and unintentionally allow
team-member check to gate owner deletions, risking privilege logic errors; use
ObjectId.equals or string comparison.
analysis.controller.ts [587-595]

Referred Code
if (analysis.userId !== req.user._id) {
  const team = await Team.findOne({ ownerId: analysis.userId });
  const isMember = team && Array.isArray((team as any).members)
    ? (team as any).members.includes(req.user._id)
    : false;
  if (!isMember) {
    return next(new CustomError("Unauthorized to delete this analysis", 403));
  }
}
Sensitive data exposure

Description: Direct access to the database via mongoose.connection.db.collection without schema-level
validation or projection can return entire documents (including internal fields) to
clients, increasing risk of sensitive data exposure; responses return 'data' verbatim
(lines 22-25).
sandbox.controller.ts [12-16]

Referred Code
  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
  // console.log("🔄 Doc: ", doc);
} catch (_) {
  // ignore cast errors
}
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit log: The delete operation logs an info message but does not clearly record a structured audit
entry with user ID, timestamp, action, and outcome as required for critical deletions.

Referred Code
logger.info("Analysis deleted", { analysisId: id, userId: req.user._id });

res.json({
  success: true,
  message: "Analysis deleted successfully",
  data: { id }
});

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Generic error UI: The frontend deletion flow catches errors but only logs to console without surfacing
actionable feedback to the user, which may lead to silent failures.

Referred Code
} catch (err) {
  console.error("Delete analysis failed", err);
  // Optional: surface error to user via toast (not implemented here)
} finally {
  setDeletingIds((prev) => ({ ...prev, [id]: false }));
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Console logging: The sandbox controller uses console logging for errors which may expose internal details
if forwarded to users or logs; ensure user-facing responses remain generic and sensitive
details are only in secure logs.

Referred Code
  console.error("Error fetching PR data:", error);
  next(new CustomError(error.message || "Failed to fetch PR data", 500));
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Unstructured logs: The use of console.log/console.error outputs unstructured logs that may include raw data
and are not structured for auditing.

Referred Code
console.log("🔄 Getting PR data: ", id);

let data = null as any;
try {
  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
  // console.log("🔄 Doc: ", doc);
} catch (_) {

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input validation: The sandbox endpoint casts req.params.id to ObjectId inside a try/catch but performs no
explicit validation or sanitization and may return entire documents directly to clients.

Referred Code
let data = null as any;
try {
  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
  // console.log("🔄 Doc: ", doc);
} catch (_) {
  // ignore cast errors
}

if (!data) {
  return next(new CustomError("PR data not found", 404));
}

return res.status(200).json({
  success: true,
  data,
});

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@shivang-16 shivang-16 closed this Nov 13, 2025
@shivang-16 shivang-16 reopened this Nov 13, 2025
@beetle-ai
Copy link
Contributor

beetle-ai bot commented Nov 13, 2025

Summary by Beetle

This pull request introduces the functionality for users to delete a completed analysis. The changes include a new backend API endpoint for handling the deletion with proper authorization and safety checks, and a corresponding delete button in the frontend analysis list.
A key safety feature prevents the deletion of any analysis that is currently in a "running" state. Additionally, an unrelated endpoint for fetching raw pull request data has been added in a new sandbox controller.
File Changes Summary (Consolidated across all commits):

File Status Changes Description
apps/api/src/controllers/analysis.controller.ts Modified +45/-0 Adds the deleteAnalysis controller. This function handles the logic for deleting an analysis, including checking if the analysis exists, verifying user authorization, and preventing the deletion of "running" analyses.
apps/api/src/controllers/sandbox.controller.ts Added +30/-0 Adds a new, unrelated controller with a getPrData function to fetch pull request data directly from the database by its ID.
apps/api/src/routes/analysis.routes.ts Modified +3/-0 Registers a new DELETE /api/analysis/:id route to expose the deleteAnalysis functionality through the API.
apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx Modified +78/-9 Updates the frontend to display a delete button on each analysis item. It implements the client-side logic for the delete action, including a confirmation dialog, API request with authorization, and a loading state.
Total Changes: 4 files changed, +156 additions, -9 deletions
Walkthrough:
sequenceDiagram
participant User
participant AnalysisListUI as Analysis List UI
participant APIServer as API Server
participant Database
User->>AnalysisListUI: Clicks delete button on an analysis
AnalysisListUI->>User: Shows confirmation dialog
User->>AnalysisListUI: Confirms deletion
AnalysisListUI->>APIServer: Sends DELETE /api/analysis/:id request
Note over APIServer: Middleware checks user authentication
APIServer->>Database: Finds analysis by ID
Database-->>APIServer: Returns analysis data
alt Analysis is "running"
APIServer-->>AnalysisListUI: Returns 400 Error ("Cannot delete a running analysis")
else Authorization Check
APIServer->>APIServer: Verifies user is owner or team member
alt Unauthorized
APIServer-->>AnalysisListUI: Returns 403 Error ("Unauthorized")
else Authorized
APIServer->>Database: Deletes analysis by ID
Database-->>APIServer: Confirms deletion
APIServer-->>AnalysisListUI: Returns 200 OK with success message
AnalysisListUI->>AnalysisListUI: Refreshes analysis list
end
Loading

Key Changes:

  • Backend API for Deletion: A new endpoint DELETE /api/analysis/:id has been created to handle the deletion of analyses.
  • Authorization Logic: The backend ensures that only the analysis owner or a member of their team can perform the deletion.
  • Safety Check: The system explicitly prevents the deletion of any analysis with a running status, requiring the user to stop it first.
  • Frontend UI: A delete button (Trash2 icon) is now visible on hover for each item in the analysis list.
  • User Experience: The deletion flow includes a browser confirmation dialog to prevent accidental deletions and a loading indicator during the API call.
    Impact Assessment:
  • Security: The implementation includes necessary authorization checks on the backend, ensuring that users can only delete analyses they own or have team access to.
  • Performance: The impact on performance is negligible, as the changes introduce a standard CRUD operation.
  • Maintainability: The code is well-structured, with clear separation of concerns between the controller, route, and frontend component. The inclusion of the unrelated sandbox.controller.ts in this PR slightly reduces the cohesiveness of the commit.
  • Testing: The new API endpoint and its associated logic (authorization, status checks) should be covered by integration tests. The frontend interactions should be validated with end-to-end tests.

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Sensitive information exposure

Description: The new PR data endpoint logs raw request parameters and errors with
console.log/console.error and directly returns the MongoDB document from
'pull_request_datas' without field whitelisting, risking sensitive information disclosure
in logs and API responses.
sandbox.controller.ts [8-28]

Referred Code
  console.log("🔄 Getting PR data: ", id);

  let data = null as any;
  try {
    data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
    // console.log("🔄 Doc: ", doc);
  } catch (_) {
    // ignore cast errors
  }

  if (!data) {
    return next(new CustomError("PR data not found", 404));
  }

  return res.status(200).json({
    success: true,
    data,
  });
} catch (error: any) {
  console.error("Error fetching PR data:", error);
  next(new CustomError(error.message || "Failed to fetch PR data", 500));
Broken authorization check

Description: Authorization checks compare ObjectId values using strict inequality and array 'includes'
on likely ObjectId arrays, which may fail due to type/reference mismatch and allow
unauthorized deletions if team lookup logic is bypassed or team doc shape differs.
analysis.controller.ts [586-595]

Referred Code
// Authorization: user must be owner or part of the team owned by analysis.userId
if (analysis.userId !== req.user._id) {
  const team = await Team.findOne({ ownerId: analysis.userId });
  const isMember = team && Array.isArray((team as any).members)
    ? (team as any).members.includes(req.user._id)
    : false;
  if (!isMember) {
    return next(new CustomError("Unauthorized to delete this analysis", 403));
  }
}
CSRF risk

Description: The client-side delete action relies solely on a bearer token and exposes a direct DELETE
to '/api/analysis/:id'; without CSRF protection for same-site contexts or additional
confirmation beyond window.confirm, it may be susceptible to CSRF if the API also accepts
cookie-based auth elsewhere.
AnalysisContent.tsx [104-131]

Referred Code
const handleDelete = async (id: string) => {
  const confirmed = typeof window !== "undefined" ? window.confirm("Delete this analysis? This cannot be undone.") : false;
  if (!confirmed) return;

  try {
    setDeletingIds((prev) => ({ ...prev, [id]: true }));
    const token = await getToken();
    if (!token || !_config.API_BASE_URL) {
      throw new Error("Missing auth token or API base URL");
    }
    const res = await fetch(`${_config.API_BASE_URL}/api/analysis/${id}`, {
      method: "DELETE",
      headers: {
        Authorization: `Bearer ${token}`,
      },
    });
    if (!res.ok) {
      const msg = `Failed to delete analysis (${res.status})`;
      throw new Error(msg);
    }
    // Refresh list; redirect useEffect will handle selection


 ... (clipped 7 lines)
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Console logging: User-facing server code uses console.log/error which can expose internal details and are
not appropriate for secure error handling; sensitive info may leak via logs.

Referred Code
console.log("🔄 Getting PR data: ", id);

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Unstructured logs: Uses console.log/error with raw data and emojis rather than structured logging, risking
leakage of sensitive PR data and reducing auditability.

Referred Code
  console.log("🔄 Getting PR data: ", id);

  let data = null as any;
  try {
    data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
    // console.log("🔄 Doc: ", doc);
  } catch (_) {
    // ignore cast errors
  }

  if (!data) {
    return next(new CustomError("PR data not found", 404));
  }

  return res.status(200).json({
    success: true,
    data,
  });
} catch (error: any) {
  console.error("Error fetching PR data:", error);
  next(new CustomError(error.message || "Failed to fetch PR data", 500));

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit details: Deletion is logged but may lack full audit context (e.g., actor identity robustness,
outcome/result code, reason) to reconstruct events.

Referred Code
logger.info("Analysis deleted", { analysisId: id, userId: req.user._id });

res.json({
  success: true,
  message: "Analysis deleted successfully",
  data: { id }
});

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Generic fetch error: Frontend delete handler throws and logs generic errors without surfacing actionable
context to the user or handling specific API error cases (e.g., 400 running, 403
unauthorized, 404 not found).

Referred Code
  if (!res.ok) {
    const msg = `Failed to delete analysis (${res.status})`;
    throw new Error(msg);
  }
  // Refresh list; redirect useEffect will handle selection
  await refreshAnalysisList();
} catch (err) {
  console.error("Delete analysis failed", err);
  // Optional: surface error to user via toast (not implemented here)
} finally {
  setDeletingIds((prev) => ({ ...prev, [id]: false }));
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
ID casting risk: The endpoint casts req.params.id to ObjectId without validating format, relying on
try/catch to ignore cast errors, which may mask bad input and lacks explicit validation.

Referred Code
let data = null as any;
try {
  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
  // console.log("🔄 Doc: ", doc);
} catch (_) {
  // ignore cast errors
}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Re-evaluate the authorization logic for deletion

The current deletion authorization logic is flawed because it assumes a user
owns only one team. It should be updated to associate analyses with a teamId for
more robust team membership checks.

Examples:

apps/api/src/controllers/analysis.controller.ts [587-595]
    if (analysis.userId !== req.user._id) {
      const team = await Team.findOne({ ownerId: analysis.userId });
      const isMember = team && Array.isArray((team as any).members)
        ? (team as any).members.includes(req.user._id)
        : false;
      if (!isMember) {
        return next(new CustomError("Unauthorized to delete this analysis", 403));
      }
    }

Solution Walkthrough:

Before:

// in deleteAnalysis controller
const analysis = await Analysis.findById(id);

// Check if user is not the direct owner
if (analysis.userId !== req.user._id) {
  // Find *any* team owned by the analysis creator
  const team = await Team.findOne({ ownerId: analysis.userId });
  
  // Check if current user is a member of that single team
  const isMember = team?.members.includes(req.user._id);

  if (!isMember) {
    return next(new CustomError("Unauthorized...", 403));
  }
}
// ... proceed with deletion

After:

// Assumes Analysis schema has an optional 'teamId'
// in deleteAnalysis controller
const analysis = await Analysis.findById(id);

const isOwner = analysis.userId === req.user._id;

let isTeamMember = false;
if (analysis.teamId) {
  const team = await Team.findById(analysis.teamId);
  isTeamMember = team?.members.includes(req.user._id) || team?.ownerId === req.user._id;
}

if (!isOwner && !isTeamMember) {
  return next(new CustomError("Unauthorized...", 403));
}
// ... proceed with deletion
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical design flaw in the authorization logic, which uses Team.findOne and incorrectly assumes a user can only own one team, making the security check unreliable.

High
Possible issue
Fix race condition in deletion

Use an atomic findOneAndDelete operation with a status check to prevent a race
condition where a running analysis could be deleted.

apps/api/src/controllers/analysis.controller.ts [597-602]

-// Prevent deletion of running analyses
-if (analysis.status === "running") {
-  return next(new CustomError("Cannot delete a running analysis. Please stop it first.", 400));
+// Atomically find and delete the analysis if it is not running.
+const deletedAnalysis = await Analysis.findOneAndDelete({
+  _id: id,
+  status: { $ne: "running" },
+});
+
+if (!deletedAnalysis) {
+  // If nothing was deleted, it's likely because the analysis is running.
+  // We already checked for its existence for authorization purposes.
+  return next(
+    new CustomError(
+      "Cannot delete a running analysis. Please stop it first.",
+      400
+    )
+  );
 }
 
-await Analysis.findByIdAndDelete(id);
-
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a race condition and proposes an atomic database operation to fix it, which significantly improves the correctness and robustness of the deletion logic.

Medium
General
Validate ObjectId before database query

Replace the generic try-catch block with an explicit
mongoose.Types.ObjectId.isValid(id) check to avoid swallowing unrelated database
errors.

apps/api/src/controllers/sandbox.controller.ts [11-16]

-try {
-  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
-  // console.log("🔄 Doc: ", doc);
-} catch (_) {
-  // ignore cast errors
+if (!mongoose.Types.ObjectId.isValid(id)) {
+  return next(new CustomError("PR data not found", 404));
 }
 
+data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
+
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out that swallowing all errors in a try-catch block is bad practice and proposes a much cleaner solution by validating the ObjectId format beforehand.

Medium
  • More

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect ObjectId comparison logic

To fix the authorization logic, use the .equals() method for comparing Mongoose
ObjectIds and Array.prototype.some() for checking team membership.

apps/api/src/controllers/analysis.controller.ts [586-595]

 // Authorization: user must be owner or part of the team owned by analysis.userId
-if (analysis.userId !== req.user._id) {
+if (!analysis.userId.equals(req.user._id)) {
   const team = await Team.findOne({ ownerId: analysis.userId });
   const isMember = team && Array.isArray((team as any).members)
-    ? (team as any).members.includes(req.user._id)
+    ? (team as any).members.some((memberId: any) => memberId.equals(req.user._id))
     : false;
   if (!isMember) {
     return next(new CustomError("Unauthorized to delete this analysis", 403));
   }
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug in the authorization logic where Mongoose ObjectIds are compared by reference instead of value, which would cause authorization to fail incorrectly.

High
Validate ID format before database query

Validate the id parameter using mongoose.Types.ObjectId.isValid() before
querying the database. If the ID is invalid, return a 400 Bad Request error
instead of a misleading 404 Not Found.

apps/api/src/controllers/sandbox.controller.ts [10-16]

-let data = null as any;
-try {
-  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
-  // console.log("🔄 Doc: ", doc);
-} catch (_) {
-  // ignore cast errors
+if (!mongoose.Types.ObjectId.isValid(id)) {
+  return next(new CustomError("Invalid ID format", 400));
 }
 
+const data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
+
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion improves API robustness by adding validation for the ID format, providing clearer and more accurate error responses to the client, distinguishing between a "bad request" and a "not found" resource.

Medium
High-level
Avoid direct database collection access

Instead of directly accessing the pull_request_datas database collection in
sandbox.controller.ts, create and use a Mongoose model. This will improve
consistency and maintainability by leveraging schema validation and established
data access patterns.

Examples:

apps/api/src/controllers/sandbox.controller.ts [12]
      data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });

Solution Walkthrough:

Before:

// apps/api/src/controllers/sandbox.controller.ts
import mongoose from "mongoose";

export const getPrData = async (req, res, next) => {
  try {
    const { id } = req.params;
    // Direct DB access
    let data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });

    if (!data) {
      return next(new CustomError("PR data not found", 404));
    }
    
    return res.status(200).json({ success: true, data });
  } catch (error) {
    next(new CustomError(error.message, 500));
  }
}

After:

// apps/api/src/models/prData.model.ts (New File)
import mongoose from 'mongoose';
const prDataSchema = new mongoose.Schema({ ... }, { collection: 'pull_request_datas' });
const PrData = mongoose.model('PrData', prDataSchema);
export default PrData;

// apps/api/src/controllers/sandbox.controller.ts
import PrData from "../models/prData.model.js";

export const getPrData = async (req, res, next) => {
  try {
    const { id } = req.params;
    // Use Mongoose model
    const data = await PrData.findById(id);

    if (!data) {
      return next(new CustomError("PR data not found", 404));
    }
    
    return res.status(200).json({ success: true, data });
  } catch (error) {
    next(new CustomError(error.message, 500));
  }
}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies direct database access in sandbox.controller.ts, which bypasses Mongoose models, introducing architectural inconsistency and potential long-term maintenance issues.

Medium
General
Use a modal for delete confirmation

Replace the native window.confirm dialog with a non-blocking modal component,
such as AlertDialog from shadcn/ui, to provide a better user experience for
delete confirmation.

apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx [104-132]

+// Add state for the dialog at the top of your component
+// const [dialogOpen, setDialogOpen] = useState(false);
+// const [analysisToDelete, setAnalysisToDelete] = useState<string | null>(null);
+
 const handleDelete = async (id: string) => {
-  const confirmed = typeof window !== "undefined" ? window.confirm("Delete this analysis? This cannot be undone.") : false;
-  if (!confirmed) return;
-
+  // This function now only handles the deletion logic after confirmation.
+  // The confirmation flow is handled by the AlertDialog component.
   try {
     setDeletingIds((prev) => ({ ...prev, [id]: true }));
     const token = await getToken();
     if (!token || !_config.API_BASE_URL) {
       throw new Error("Missing auth token or API base URL");
     }
     const res = await fetch(`${_config.API_BASE_URL}/api/analysis/${id}`, {
       method: "DELETE",
       headers: {
         Authorization: `Bearer ${token}`,
       },
     });
     if (!res.ok) {
-      const msg = `Failed to delete analysis (${res.status})`;
+      const data = await res.json().catch(() => ({}));
+      const msg = data.message || `Failed to delete analysis (${res.status})`;
       throw new Error(msg);
     }
     // Refresh list; redirect useEffect will handle selection
     await refreshAnalysisList();
   } catch (err) {
     console.error("Delete analysis failed", err);
-    // Optional: surface error to user via toast (not implemented here)
+    // Optional: surface error to user via toast
   } finally {
     setDeletingIds((prev) => ({ ...prev, [id]: false }));
   }
 };
 
+/*
+Then, wrap your delete button in an AlertDialog:
+(This is a conceptual example, you'll need to import AlertDialog components)
+
+<AlertDialog>
+  <AlertDialogTrigger asChild>
+    <Button ...>
+      <Trash2 className="h-4 w-4" />
+    </Button>
+  </AlertDialogTrigger>
+  <AlertDialogContent>
+    <AlertDialogHeader>
+      <AlertDialogTitle>Are you sure?</AlertDialogTitle>
+      <AlertDialogDescription>
+        This will permanently delete the analysis. This action cannot be undone.
+      </AlertDialogDescription>
+    </AlertDialogHeader>
+    <AlertDialogFooter>
+      <AlertDialogCancel>Cancel</AlertDialogCancel>
+      <AlertDialogAction onClick={() => handleDelete(analysis._id)}>
+        Delete
+      </AlertDialogAction>
+    </AlertDialogFooter>
+  </AlertDialogContent>
+</AlertDialog>
+*/
+
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion proposes a valid UX improvement by replacing the blocking window.confirm with a non-blocking modal dialog, which aligns better with modern web application design.

Low
  • More

@shivang-16 shivang-16 closed this Nov 18, 2025
@shivang-16 shivang-16 reopened this Nov 18, 2025
@beetle-ai
Copy link
Contributor

beetle-ai bot commented Nov 18, 2025

Summary by Beetle

This PR introduces a comprehensive delete analysis functionality across the full stack, enabling users to safely remove analyses they own or have team access to. The implementation includes backend authorization checks, frontend UI integration with confirmation dialogs, and safeguards to prevent deletion of running analyses. Additionally, a new sandbox controller endpoint is added for retrieving pull request data.

File Changes Summary (Consolidated across all commits):

File Status Changes Description
apps/api/src/controllers/analysis.controller.ts Modified +45/-0 Added deleteAnalysis controller with authorization checks (owner or team member), prevents deletion of running analyses, and includes proper logging
apps/api/src/controllers/sandbox.controller.ts Added +30/-0 New controller implementing getPrData endpoint to fetch pull request data from MongoDB collection by ID
apps/api/src/routes/analysis.routes.ts Modified +3/-0 Registered DELETE route for analysis deletion at /:id endpoint with base authentication middleware
apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx Modified +78/-9 Added delete button with trash icon in analysis list items, confirmation dialog, loading states, disabled state for running analyses, and automatic list refresh after deletion

Total Changes: 4 files changed, +156 additions, -9 deletions

Walkthrough:

sequenceDiagram
participant User
participant Frontend as AnalysisContent.tsx
participant API as analysis.routes.ts
participant Controller as analysis.controller.ts
participant DB as MongoDB
User->>Frontend: Click delete button
Frontend->>User: Show confirmation dialog
User->>Frontend: Confirm deletion
Frontend->>Frontend: Set deleting state
Frontend->>API: DELETE /api/analysis/:id
Note over Frontend,API: Authorization: Bearer ***
API->>Controller: deleteAnalysis()
Controller->>DB: Find analysis by ID
DB-->>Controller: Return analysis
Controller->>Controller: Check authorization<br/>(owner or team member)
Controller->>Controller: Validate status<br/>(not running)
Controller->>DB: Delete analysis
DB-->>Controller: Deletion confirmed
Controller-->>API: Success response
API-->>Frontend: 200 OK
Frontend->>Frontend: Refresh analysis list
Frontend->>Frontend: Clear deleting state
Frontend-->>User: Updated list displayed
Loading

Key Changes:

  • Backend Authorization: Implements robust authorization logic checking if the user is either the analysis owner or a member of the owner's team before allowing deletion
  • Safety Guards: Prevents deletion of analyses with "running" status, requiring users to stop the analysis first to avoid data corruption or incomplete operations
  • Frontend UX: Delete button appears on hover with trash icon, includes loading spinner during deletion, and is disabled for running analyses
  • Confirmation Flow: User confirmation dialog prevents accidental deletions with clear warning that the action cannot be undone
  • Automatic Refresh: After successful deletion, the analysis list automatically refreshes and redirects to an appropriate analysis if the deleted one was currently selected
  • New Sandbox Endpoint: Added getPrData controller for retrieving pull request data, supporting future PR analysis features
    Impact Assessment:
  • Security: ✅ Strong authorization checks ensure users can only delete analyses they own or have team access to. Prevents unauthorized deletion attempts with 403 responses. Running analyses are protected from deletion.
  • Performance: ✅ Minimal impact. Single database query for authorization check and deletion. Frontend uses optimistic UI updates with loading states. No performance concerns identified.
  • Maintainability: ✅ Well-structured code following existing patterns. Clear separation of concerns between controller logic and route definitions. Proper error handling and logging throughout. The sandbox controller is isolated and doesn't affect existing functionality.
  • Testing: ⚠️ No test files included in this PR. Recommended to add:
  • Unit tests for deleteAnalysis controller covering authorization scenarios
  • Integration tests for the DELETE endpoint
  • Frontend tests for delete button interactions and confirmation flow
  • Edge case testing for concurrent deletion attempts

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Sensitive information exposure

Description: The endpoint logs sensitive request details to console (e.g., "🔄 Getting PR data" and full
error objects), which can expose identifiers and internal errors in production logs and
facilitate information disclosure.
sandbox.controller.ts [8-28]

Referred Code
  console.log("🔄 Getting PR data: ", id);

  let data = null as any;
  try {
    data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
    // console.log("🔄 Doc: ", doc);
  } catch (_) {
    // ignore cast errors
  }

  if (!data) {
    return next(new CustomError("PR data not found", 404));
  }

  return res.status(200).json({
    success: true,
    data,
  });
} catch (error: any) {
  console.error("Error fetching PR data:", error);
  next(new CustomError(error.message || "Failed to fetch PR data", 500));
CSRF risk

Description: Using window.confirm for destructive actions without CSRF mitigation on the subsequent
fetch may allow CSRF if the API relies solely on bearer tokens in cookies; ensure tokens
are not sent automatically via cookies or add CSRF protections.
AnalysisContent.tsx [105-113]

Referred Code
const confirmed = typeof window !== "undefined" ? window.confirm("Delete this analysis? This cannot be undone.") : false;
if (!confirmed) return;

try {
  setDeletingIds((prev) => ({ ...prev, [id]: true }));
  const token = await getToken();
  if (!token || !_config.API_BASE_URL) {
    throw new Error("Missing auth token or API base URL");
  }
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Console error leak: User-facing path may be fine but internal errors are printed with console.error which can
surface stack traces or sensitive details in logs accessible beyond secure channels.

Referred Code
console.error("Error fetching PR data:", error);
next(new CustomError(error.message || "Failed to fetch PR data", 500));

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Unstructured logs: The use of console.log with raw request parameter values produces unstructured logs and
may expose identifiers; structured logger should be used without sensitive data.

Referred Code
console.log("🔄 Getting PR data: ", id);

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit details: Deletion is logged but may lack structured audit context like timestamp, outcome status,
and actor details beyond userId; verify centralized structured audit logging is used.

Referred Code
logger.info("Analysis deleted", { analysisId: id, userId: req.user._id });

res.json({
  success: true,
  message: "Analysis deleted successfully",
  data: { id }
});

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Weak error handling: The controller uses console logging and swallows cast errors without logging or structured
context, reducing observability for edge cases.

Referred Code
try {
  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
  // console.log("🔄 Doc: ", doc);
} catch (_) {
  // ignore cast errors
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Authorization nuance: Authorization checks rely on comparing userId and team membership but do not handle cases
like missing team doc or objectId string/Buffer equality nuances; verify type-safe checks
and null cases.

Referred Code
// Authorization: user must be owner or part of the team owned by analysis.userId
if (analysis.userId !== req.user._id) {
  const team = await Team.findOne({ ownerId: analysis.userId });
  const isMember = team && Array.isArray((team as any).members)
    ? (team as any).members.includes(req.user._id)
    : false;
  if (!isMember) {
    return next(new CustomError("Unauthorized to delete this analysis", 403));
  }
}

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Comment on lines +587 to +591
if (analysis.userId !== req.user._id) {
const team = await Team.findOne({ ownerId: analysis.userId });
const isMember = team && Array.isArray((team as any).members)
? (team as any).members.includes(req.user._id)
: false;
Copy link
Contributor

Choose a reason for hiding this comment

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

The authorization check compares analysis.userId with req.user._id using strict inequality, but the type consistency isn't guaranteed. If analysis.userId is stored as a string and req.user._id is an ObjectId (or vice versa), the comparison will fail even for the owner, forcing the code to check team membership unnecessarily.

Confidence: 4/5

Suggested Fix
Suggested change
if (analysis.userId !== req.user._id) {
const team = await Team.findOne({ ownerId: analysis.userId });
const isMember = team && Array.isArray((team as any).members)
? (team as any).members.includes(req.user._id)
: false;
if (analysis.userId.toString() !== req.user._id.toString()) {
const team = await Team.findOne({ ownerId: analysis.userId });
const isMember = team && Array.isArray((team as any).members)
? (team as any).members.some(memberId => memberId.toString() === req.user._id.toString())
: false;

Convert both IDs to strings for consistent comparison. Also use .some() with string comparison instead of .includes() to ensure type-safe membership checking.

📍 This suggestion applies to lines 587-591

Comment on lines +589 to +591
const isMember = team && Array.isArray((team as any).members)
? (team as any).members.includes(req.user._id)
: false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using (team as any).members bypasses TypeScript's type checking and could lead to runtime errors if the Team model doesn't have a members property or if its structure changes. This makes the code fragile and harder to maintain.

Confidence: 4/5

Suggested Fix

Consider properly typing the Team model or adding a type guard:

const isMember = team && 'members' in team && Array.isArray(team.members)
? team.members.some(memberId => memberId.toString() === req.user._id.toString())
: false;

Or better yet, ensure the Team model is properly typed in your codebase and use that type instead of any.

📍 This suggestion applies to lines 589-591

return next(new CustomError("Cannot delete a running analysis. Please stop it first.", 400));
}

await Analysis.findByIdAndDelete(id);
Copy link
Contributor

Choose a reason for hiding this comment

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

The deletion operation only removes the Analysis document but doesn't handle potential related data. If there are associated resources (analysis results, logs, files, etc.), they may become orphaned. Consider whether cascade deletion or cleanup of related resources is needed.

Confidence: 3/5

Suggested Fix

Consider implementing a transaction or cascade delete pattern:

// Option 1: Use a transaction
const session = await mongoose.startSession();
session.startTransaction();
try {
await Analysis.findByIdAndDelete(id, { session });
// Delete related resources
await AnalysisResult.deleteMany({ analysisId: id }, { session });
await session.commitTransaction();
} catch (error) {
await session.abortTransaction();
throw error;
} finally {
session.endSession();
}
// Option 2: Add cascade delete hooks in the Analysis model

Review your data model to determine if related resources need cleanup.

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent race condition during deletion

To prevent a race condition, use a single atomic findOneAndDelete operation that
includes a status check in the query, ensuring a running analysis is not
deleted.

apps/api/src/controllers/analysis.controller.ts [580-602]

-// Find the analysis
-const analysis = await Analysis.findById(id);
+// Find the analysis for authorization checks first
+const analysis = await Analysis.findById(id).select('userId').lean();
 if (!analysis) {
   return next(new CustomError("Analysis not found", 404));
 }
 
 // Authorization: user must be owner or part of the team owned by analysis.userId
-if (analysis.userId !== req.user._id) {
+if (analysis.userId.toString() !== req.user._id.toString()) {
   const team = await Team.findOne({ ownerId: analysis.userId });
   const isMember = team && Array.isArray((team as any).members)
-    ? (team as any).members.includes(req.user._id)
+    ? (team as any).members.some((memberId: any) => memberId.toString() === req.user._id.toString())
     : false;
   if (!isMember) {
     return next(new CustomError("Unauthorized to delete this analysis", 403));
   }
 }
 
-// Prevent deletion of running analyses
-if (analysis.status === "running") {
-  return next(new CustomError("Cannot delete a running analysis. Please stop it first.", 400));
+// Atomically delete the analysis only if it is not running
+const deletedAnalysis = await Analysis.findOneAndDelete({
+  _id: id,
+  status: { $ne: "running" },
+});
+
+if (!deletedAnalysis) {
+  // If not found, it's either already deleted (ok) or it's running.
+  // Check if it exists and is running.
+  const isRunning = await Analysis.exists({ _id: id, status: "running" });
+  if (isRunning) {
+    return next(new CustomError("Cannot delete a running analysis. Please stop it first.", 400));
+  }
+  // If it's not running, it was likely deleted by another request. We can treat this as a success.
 }
 
-await Analysis.findByIdAndDelete(id);
-
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a race condition where a running analysis could be deleted. The proposed fix using an atomic findOneAndDelete operation is the correct approach to ensure data integrity.

Medium
General
Validate object ID before querying

Validate the id parameter using mongoose.Types.ObjectId.isValid(id) before
querying the database to avoid silently ignoring casting errors and improve code
clarity.

apps/api/src/controllers/sandbox.controller.ts [10-20]

-let data = null as any;
-try {
-  data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
-  // console.log("🔄 Doc: ", doc);
-} catch (_) {
-  // ignore cast errors
+if (!mongoose.Types.ObjectId.isValid(id)) {
+  return next(new CustomError("PR data not found", 404));
 }
+
+const data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
 
 if (!data) {
   return next(new CustomError("PR data not found", 404));
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion improves code quality by replacing a try-catch block that swallows errors with explicit ObjectId validation, making the code cleaner and more robust.

Low
Use a modal for delete confirmation

Replace the browser-native window.confirm dialog with a more user-friendly,
non-blocking confirmation modal to improve the user experience and align with
modern UI practices.

apps/web/app/(root)/analysis/[repoId]/_components/AnalysisContent.tsx [104-132]

 const handleDelete = async (id: string) => {
+  // This should be replaced with a call to open a custom confirmation modal.
+  // For example: `openConfirmationModal({ onConfirm: () => performDelete(id) })`
+  // The implementation below is kept for demonstration of the flow.
   const confirmed = typeof window !== "undefined" ? window.confirm("Delete this analysis? This cannot be undone.") : false;
   if (!confirmed) return;
 
   try {
     setDeletingIds((prev) => ({ ...prev, [id]: true }));
     const token = await getToken();
     if (!token || !_config.API_BASE_URL) {
       throw new Error("Missing auth token or API base URL");
     }
     const res = await fetch(`${_config.API_BASE_URL}/api/analysis/${id}`, {
       method: "DELETE",
       headers: {
         Authorization: `Bearer ${token}`,
       },
     });
     if (!res.ok) {
-      const msg = `Failed to delete analysis (${res.status})`;
-      throw new Error(msg);
+      const errorData = await res.json().catch(() => ({ message: `Failed to delete analysis (${res.status})` }));
+      throw new Error(errorData.message || `Failed to delete analysis (${res.status})`);
     }
     // Refresh list; redirect useEffect will handle selection
     await refreshAnalysisList();
   } catch (err) {
     console.error("Delete analysis failed", err);
-    // Optional: surface error to user via toast (not implemented here)
+    // Optional: surface error to user via toast, e.g., toast.error((err as Error).message)
   } finally {
     setDeletingIds((prev) => ({ ...prev, [id]: false }));
   }
 };
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out that window.confirm provides a poor user experience. Replacing it with a custom modal is a valid UI/UX improvement, though it is a stylistic change rather than a functional bug fix.

Low
  • More

Comment on lines +10 to +16
let data = null as any;
try {
data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
// console.log("🔄 Doc: ", doc);
} catch (_) {
// ignore cast errors
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Silent Error Swallowing with Unsafe Type Casting
This code has two critical issues:

  1. Type Safety Violation: Using any type (let data = null as any;) defeats TypeScript's type safety and can lead to runtime errors when accessing properties on the returned data.
  2. Silent Error Handling: The empty catch block (catch (_) { // ignore cast errors }) silently swallows all errors, including legitimate database connection issues, network failures, or permission errors. This makes debugging extremely difficult and could hide critical problems.
    Impact:
  • Runtime errors from undefined property access won't be caught at compile time
  • Database connection failures or other critical errors will be silently ignored
  • Debugging production issues will be nearly impossible

Confidence: 5/5

Suggested Fix
Suggested change
let data = null as any;
try {
data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
// console.log("🔄 Doc: ", doc);
} catch (_) {
// ignore cast errors
}
let data = null;
try {
data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
} catch (error) {
console.error("Error querying PR data:", error);
// Only ignore ObjectId cast errors, not all errors
if (error instanceof Error && !error.message.includes('Argument passed in must be')) {
return next(new CustomError("Database error while fetching PR data", 500));
}

Changes:

  1. Remove as any type assertion to maintain type safety
  2. Log the actual error for debugging purposes
  3. Only ignore specific ObjectId cast errors, not all errors
  4. Return proper error response for genuine database failures

📍 This suggestion applies to lines 10-16


let data = null as any;
try {
data = await mongoose.connection.db?.collection('pull_request_datas').findOne({ _id: new mongoose.Types.ObjectId(id) });
Copy link
Contributor

Choose a reason for hiding this comment

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

Direct Database Access Bypasses Model Layer
The code directly accesses the MongoDB collection (mongoose.connection.db?.collection('pull_request_datas')) instead of using a Mongoose model. This approach:

  1. Bypasses Schema Validation: Any schema validations, virtuals, or middleware defined in a Mongoose model won't be applied
  2. Inconsistent Data Access Pattern: If other parts of the codebase use models, this creates inconsistency
  3. Type Safety Issues: Direct collection access returns plain objects without TypeScript types
  4. Missing Model Features: Loses benefits like population, virtuals, and instance methods
    Recommendation: Consider creating a Mongoose model for pull_request_datas collection to maintain consistency and leverage Mongoose features. If direct access is intentional for performance reasons, document why this approach was chosen.

Confidence: 4/5

router.put("/:id/status", baseAuth, updateAnalysisStatus);
router.get("/:id/logs", baseAuth, getRepositoryAnalysisLogs);
// Delete an analysis by ID
router.delete("/:id", baseAuth, deleteAnalysis);
Copy link
Contributor

Choose a reason for hiding this comment

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

Security Issue: Missing Authorization Check on Delete Route
The DELETE route is missing the checkAnalysisAccess middleware that other analysis-modifying routes use (like the stop route on line 40). This could allow users to delete analyses they don't have permission to access. The baseAuth middleware only verifies authentication, not authorization to access the specific analysis.

Confidence: 5/5

Suggested Fix
Suggested change
router.delete("/:id", baseAuth, deleteAnalysis);
router.delete("/:id", baseAuth, checkAnalysisAccess, deleteAnalysis);

Add the checkAnalysisAccess middleware between baseAuth and deleteAnalysis to ensure users can only delete analyses they have permission to access. This matches the authorization pattern used by other analysis-modifying routes in this file.

}, []);

const handleDelete = async (id: string) => {
const confirmed = typeof window !== "undefined" ? window.confirm("Delete this analysis? This cannot be undone.") : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

The window.confirm() dialog provides a poor user experience and is not accessible. It's a blocking browser dialog that doesn't match the modern UI patterns used elsewhere in the application (like the Tooltip component).

Confidence: 4/5

Suggested Fix

Consider replacing the native window.confirm() with a proper confirmation dialog component that matches your design system. This would provide better UX, accessibility, and consistency with the rest of the application. You could use a Dialog/AlertDialog component from your UI library (similar to how you're using Tooltip).
Example approach:

  • Add a state for tracking which analysis is pending deletion
  • Show a Dialog component with proper styling and accessibility
  • Include clear "Cancel" and "Delete" buttons
  • Provide better messaging about the consequences

Comment on lines +127 to +128
console.error("Delete analysis failed", err);
// Optional: surface error to user via toast (not implemented here)
Copy link
Contributor

Choose a reason for hiding this comment

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

Error handling silently logs to console without providing user feedback. When deletion fails, users won't know what went wrong or that the operation failed at all. The comment acknowledges this limitation but it should be addressed.

Confidence: 5/5

Suggested Fix

Implement proper error feedback to users when deletion fails. Options include:

  • Add a toast notification system to show error messages
  • Display an inline error message near the delete button
  • Use an alert dialog to inform the user of the failure
    This is important for user experience - users need to know when operations fail and why, especially for destructive actions like deletion.

📍 This suggestion applies to lines 127-128

Comment on lines +198 to +199
</TooltipTrigger>

Copy link
Contributor

Choose a reason for hiding this comment

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

The TooltipTrigger is present but there's no TooltipContent component to display the actual tooltip text. Line 199 is empty and line 200 just closes the Tooltip. This means users won't see any helpful text when hovering over the delete button.

Confidence: 5/5

Suggested change
</TooltipTrigger>
<TooltipTrigger asChild>
<Button
variant="ghost"
size="icon"
className="text-muted-foreground"
aria-label="Delete analysis"
disabled={analysis.status === "running" || !!deletingIds[analysis._id]}
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
handleDelete(analysis._id);
}}
>
{deletingIds[analysis._id] ? (
<Loader2 className="h-4 w-4 animate-spin" />
) : (
<Trash2 className="h-4 w-4" />
)}
</Button>
</TooltipTrigger>
<TooltipContent>
<p>Delete analysis</p>
</TooltipContent>
</Tooltip>

Add the missing TooltipContent component to provide helpful text to users when they hover over the delete button.

📍 This suggestion applies to lines 198-199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Review effort 2/5 Review effort 3/5 size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants