Remove db references from API

[scazan]

Moves more DB logic into the Common library pulling logic out of the API handlers.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
api	✅ Ready (Inspect)	Visit Preview	Jul 26, 2025 9:20am
app	❌ Failed (Inspect)		Jul 26, 2025 9:20am

1 Skipped Deployment

Name	Status	Preview	Updated (UTC)
apps-workshop	⬜️ Ignored (Inspect)	Visit Preview	Jul 26, 2025 9:20am

[claude]

Claude finished @scazan's task —— View job

---

PR Review Complete ✅

This is a well-executed refactoring that successfully separates database logic from API handlers into reusable common services.

Review Summary

Overall Assessment: ✅ APPROVED

This refactoring follows good architectural patterns and improves code organization. The changes are substantial but well-structured.

---

Detailed Analysis

✅ Strengths

1. Clean Architecture Pattern

• Excellent separation of concerns: API layer handles HTTP/tRPC concerns, common layer handles business logic
• Database operations properly abstracted into dedicated service functions
• Consistent file organization and naming conventions

2. Code Quality

• Well-structured TypeScript interfaces with proper typing
• Consistent error handling patterns using custom error classes
• Good use of Drizzle ORM query builder with proper type safety

3. Reusability & DRY

• Services can now be reused across different parts of the application
• Eliminates code duplication between API handlers
• Clear function signatures with proper input/output types

4. Error Handling

• Consistent use of custom error classes (CommonError, UnauthorizedError, NotFoundError)
• Proper error propagation from common services to API handlers
• Good error messages and context preservation

---

⚠️ Issues to Address

Security Concerns

1. SQL Injection Prevention (packages/common/src/services/organization/getOrganizationsByProfile.ts:30)

x: sql<number>`ST_X(${locations.location})`.as('x'),
y: sql<number>`ST_Y(${locations.location})`.as('y'),

✅ Safe: Uses Drizzle's typed SQL template literals, which are parameterized

2. Authorization Checks

• ✅ Good: Authorization properly checked in createPostInOrganization.ts:21-28
• ✅ Good: Post ownership validated in deletePost.ts:11-29

Potential Bugs

1. Race Condition in Reactions (packages/common/src/services/reactions/addReaction.ts:13-27)

// First, remove any existing reaction
await db.delete(postReactions).where(...)
// Then add the new reaction  
await db.insert(postReactions).values(...)

⚠️ Issue: Not atomic - could result in lost reactions if multiple requests occur simultaneously
Recommendation: Use a transaction or upsert pattern

2. Type Safety Issue (packages/common/src/services/posts/createPostInOrganization.ts:68-69)

// @ts-expect-error - We check for this existence first. TODO: find the source of this TS error
storageObject?.name

⚠️ Issue: Suppressed TypeScript error indicates potential runtime issues
Recommendation: Fix the underlying type definition instead of suppressing

3. Inconsistent Error Handling (services/api/src/routers/organization/reactions.ts:69-71)

} catch (e) {
  throw new CommonError('Failed to toggle reaction');
}

⚠️ Issue: Original error information is lost, making debugging difficult
Recommendation: Log original error or include in error message

Performance Considerations

1. N+1 Query Potential (packages/common/src/services/organization/getOrganizationsByProfile.ts:7-50)

• ✅ Good: Uses proper joins with with clauses to avoid N+1 queries
• Complex query but well-optimized with Drizzle's relation loading

2. Missing Pagination (packages/common/src/services/posts/listRelatedOrganizationPosts.ts:105)

• ⚠️ Issue: listRelatedOrganizationPosts lacks pagination, but listAllRelatedOrganizationPosts has it
• Recommendation: Consider adding pagination to prevent large result sets

Code Quality Issues

1. Debugging Code Left In (packages/common/src/services/posts/createPostInOrganization.ts:93)

console.log('ERROR', error);

⚠️ Issue: Debug statement should use proper logging
Recommendation: Use structured logging instead of console.log

2. Type Assertions (packages/common/src/services/organization/inviteUsers.ts:51,76)

(authUser.currentOrganization as any)?.profile

⚠️ Issue: Type assertions indicate type definition issues
Recommendation: Fix underlying type definitions

---

✅ Best Practices Followed

1. Proper Database Patterns: Good use of transactions where needed
2. Input Validation: Proper Zod schemas in API handlers
3. Error Boundaries: Consistent error handling patterns
4. Type Safety: Strong TypeScript usage throughout
5. Documentation: Clear function interfaces and parameter types

---

Recommendations for Follow-up

1. Fix the reaction race condition with proper transactions
2. Resolve TypeScript suppressions by fixing underlying type definitions
3. Add structured logging instead of console.log statements
4. Consider adding pagination to all list operations for consistency
5. Add unit tests for the new common services to ensure reliability

---

Test Recommendations

Given this is a significant refactoring, I recommend:

1. Integration tests to verify API endpoints still work correctly
2. Unit tests for the new common services
3. Testing edge cases around authorization and error handling

Overall, this is a solid refactoring that improves the codebase architecture significantly. The issues identified are mostly minor and can be addressed in follow-up PRs.

---

[claude]

Claude finished @scazan's task —— View job

---

PR Review Complete ✅

This is a solid architectural refactoring that successfully separates database logic from API handlers into reusable common services. The changes follow good patterns but there are several issues to address.

Review Tasks:

• Analyze the architectural changes and separation of concerns
• Review code quality and TypeScript best practices
• Identify potential bugs and error handling issues
• Check for security vulnerabilities
• Assess performance implications
• Provide constructive feedback and recommendations

---

Overall Assessment: ✅ APPROVED with recommendations

This refactoring successfully improves code organization and reusability. The architectural pattern is sound, but several issues should be addressed in follow-up work.

---

✅ Strengths

1. Excellent Architecture Pattern

• Clean separation between API layer (HTTP/tRPC concerns) and business logic (common services)
• Consistent service interfaces with proper TypeScript typing
• Database operations properly abstracted into dedicated service functions
• Good reusability - services can be used across different application parts

2. Code Quality

• Well-structured interfaces (CreatePostInOrganizationOptions, ToggleReactionOptions, etc.)
• Consistent error handling using custom error classes (CommonError, UnauthorizedError, NotFoundError)
• Good use of Drizzle ORM with type safety and proper query building
• Proper authorization checks (e.g., getOrgAccessUser in createPostInOrganization.ts:21-28)

3. Performance Optimizations

• ✅ Efficient queries: Uses joins with with clauses to avoid N+1 queries (getOrganizationsByProfile.ts:7-50)
• ✅ Parallel execution: Uses Promise.all() for concurrent operations (createPostInOrganization.ts:81)

---

⚠️ Critical Issues to Address

1. Race Condition in Reactions

File: packages/common/src/services/reactions/addReaction.ts:12-29

await db.transaction(async (tx) => {
  // First, remove any existing reaction
  await tx.delete(postReactions).where(...)
  // Then add the new reaction  
  await tx.insert(postReactions).values(...)
});

✅ Actually Good: This IS properly wrapped in a transaction, making it atomic. My initial concern was unfounded.

2. Debug Code Left in Production

File: packages/common/src/services/posts/createPostInOrganization.ts:92

console.log('ERROR', error);

⚠️ Issue: Debug statements should use structured logging
Recommendation: Replace with proper logging infrastructure

3. TypeScript Error Suppression

File: packages/common/src/services/organization/inviteUsers.ts:51,76 (based on previous review)
Issue: Type assertions indicate underlying type definition problems
Recommendation: Fix root cause in type definitions rather than using suppressions

4. Error Context Loss

File: services/api/src/routers/organization/reactions.ts:69-71

} catch (e) {
  throw new CommonError('Failed to toggle reaction');
}

⚠️ Issue: Original error information is discarded, making debugging difficult
Recommendation: Include original error context or log it before throwing

---

🔒 Security Analysis

SQL Injection Check

File: packages/common/src/services/organization/getOrganizationsByProfile.ts:30-31

x: sql<number>`ST_X(${locations.location})`.as('x'),
y: sql<number>`ST_Y(${locations.location})`.as('y'),

✅ Safe: Uses Drizzle's parameterized SQL template literals

Authorization Verification

• ✅ Good: Proper authorization in createPostInOrganization.ts:21-28
• ✅ Good: Post ownership validation in deletePost.ts:11-29
• ✅ Good: User access validation throughout reaction handlers

---

🚀 Performance Considerations

Query Optimization

• ✅ Excellent: Complex queries use proper joins to prevent N+1 issues
• ✅ Good: Efficient use of Drizzle's relation loading system

Pagination

• ✅ Good: listAllRelatedOrganizationPosts has proper cursor-based pagination
• ⚠️ Inconsistent: Some list functions may lack pagination (verify listRelatedOrganizationPosts vs listAllRelatedOrganizationPosts)

---

📝 Code Quality Issues

1. Inconsistent Error Handling

Some handlers properly preserve error context while others don't. Standardize error handling patterns.

2. Type Safety

The codebase generally has good TypeScript usage, but some suppressions indicate room for improvement in type definitions.

3. Testing Recommendations

Given this is a significant refactoring:

• Add unit tests for the new common services
• Integration tests to verify API endpoints work correctly
• Test edge cases around authorization and error scenarios

---

🎯 Recommendations for Follow-up

1. Replace console.log with structured logging
2. Resolve TypeScript suppressions by fixing underlying type definitions
3. Standardize error handling to preserve context while providing user-friendly messages
4. Add comprehensive tests for the new service layer
5. Consider adding monitoring/observability for the new service boundaries

---

📋 Summary

This refactoring successfully achieves its goal of separating concerns and improving code organization. The architectural pattern is sound and will improve maintainability. The issues identified are mostly minor and don't block the core functionality.

Impact: Positive architectural improvement that sets up the codebase for better maintainability and reusability.

Risk Level: Low - issues are primarily code quality improvements rather than functional bugs.

---