Skip to content

Conversation

@hbjORbj
Copy link
Contributor

@hbjORbj hbjORbj commented Nov 22, 2025

What does this PR do?

This PR breaks two circular dependencies in the workflows and billing modules:

  1. reminderScheduler ↔ credit-service: Inverts the dependency so billing calls workflows (not vice versa) and introduces a repository pattern for Prisma queries
  2. messageDispatcher ↔ credit-service: Implements dependency injection to thread credit checking through the call chain, eliminating the need for workflows to import billing

Circular Dependency #1: reminderScheduler ↔ credit-service

Before:

  • credit-service imported cancelScheduledMessagesAndScheduleEmails from reminderScheduler
  • reminderScheduler dynamically imported CreditService to check credits
  • This created a cycle: credit-service → reminderScheduler → credit-service

After:

  • credit-service pre-computes userIdsWithoutCredits before calling cancelScheduledMessagesAndScheduleEmails
  • reminderScheduler no longer imports CreditService
  • Created WorkflowReminderRepository to encapsulate Prisma queries (fixing repository pattern violations)
  • One-way dependency: billing → workflows ✅

Circular Dependency #2: messageDispatcher ↔ credit-service (4-file cycle)

Before:

credit-service:5 → reminderScheduler:27 → smsReminderManager:27 → messageDispatcher:32,80 → credit-service

After:

  • Added optional creditCheckFn parameter to messageDispatcher functions
  • Threaded creditCheckFn through: scheduleWorkflowReminders → processWorkflowStep → scheduleSMSReminder/scheduleWhatsappReminder → messageDispatcher
  • When creditCheckFn is provided, use it; otherwise fall back to dynamic CreditService import for backward compatibility
  • Breaks the workflows → billing import while preserving immediate fallback behavior ✅

Key Changes

Files Modified:

  • credit-service.ts: Pre-compute userIdsWithoutCredits before calling cancelScheduledMessagesAndScheduleEmails
  • reminderScheduler.ts: Accept userIdsWithoutCredits parameter, add creditCheckFn to ScheduleWorkflowRemindersArgs, thread through processWorkflowStep
  • messageDispatcher.ts: Accept optional creditCheckFn parameter, use it if provided or fall back to dynamic import
  • smsReminderManager.ts: Thread creditCheckFn through scheduleSMSReminder
  • whatsappReminderManager.ts: Thread creditCheckFn through scheduleWhatsappReminder

Files Created:

  • WorkflowReminderRepository.ts: New repository to encapsulate Prisma queries for workflow reminders (fixes repository pattern violations)

Testing

All existing tests pass:

  • ✅ Type checks: No new type errors in modified files
  • ✅ Lint checks: 0 errors (646 warnings are pre-existing)
  • ✅ Unit tests: 415 test files passed, 3907 tests passed

Manual Testing Needed:

  1. Credit exhaustion flow: When a user/team runs out of SMS credits, verify that scheduled SMS reminders are cancelled and converted to email reminders
  2. Backward compatibility: Verify that API endpoints (scheduleSMSReminders, scheduleWhatsappReminders) still work without passing creditCheckFn (should use fallback)
  3. Immediate sends: Verify that immediate SMS sends (sendSmsOrFallbackEmail) correctly check credits before sending

Human Review Checklist

⚠️ Important: This PR includes changes from two separate architectural fixes. Ideally these should be in separate PRs, but they're combined here due to branching from the wrong base.

Critical items to review:

  1. Verify both circular dependencies are actually broken (no workflows → billing imports in messageDispatcher)
  2. Check that creditCheckFn is correctly threaded through all call paths (reminderScheduler → smsReminderManager/whatsappReminderManager → messageDispatcher)
  3. Verify backward compatibility: API endpoints can omit creditCheckFn and rely on fallback dynamic import
  4. Confirm WorkflowReminderRepository correctly encapsulates Prisma queries (no Prisma outside repositories)
  5. Check that userIdsWithoutCredits is correctly computed in credit-service before calling cancelScheduledMessagesAndScheduleEmails
  6. Verify no breaking changes to existing callers (all parameters are optional)
  7. Confirm tests were updated to match new function signatures

Potential risks:

  • Complex dependency injection threading across 4+ layers
  • Fallback behavior relies on dynamic imports (could mask issues)
  • No explicit tests for the DI path (relies on existing tests)
  • Repository pattern may not be consistently applied elsewhere in codebase

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox. N/A - Internal refactoring, no API changes
  • I confirm automated tests are in place that prove my fix is effective or that my feature works. (Existing tests pass, no new tests needed for refactoring)

How should this be tested?

Environment Setup:

  • Ensure SMS credits are enabled: IS_SMS_CREDITS_ENABLED=true
  • Configure Twilio credentials for SMS testing

Test Scenarios:

  1. Credit exhaustion (team):

    • Create a team with scheduled SMS workflow reminders
    • Exhaust the team's SMS credits
    • Verify scheduled SMS reminders are cancelled and converted to email reminders
  2. Credit exhaustion (user):

    • Create a user with scheduled SMS workflow reminders
    • Exhaust the user's SMS credits
    • Verify scheduled SMS reminders are cancelled and converted to email reminders
  3. Backward compatibility:

    • Trigger the scheduleSMSReminders CRON job
    • Verify it works without errors (uses fallback dynamic import)
  4. Immediate sends:

    • Create a workflow with immediate SMS send (NEW_EVENT trigger)
    • Book an event
    • Verify SMS is sent if credits available, email fallback if not

Expected Results:

  • No circular dependency errors in build/runtime
  • SMS reminders correctly cancelled when credits exhausted
  • Email fallbacks sent when SMS cannot be sent
  • No breaking changes to existing functionality

Link to Devin run: https://app.devin.ai/sessions/001329399e164c19b682ea5d209e8f1c
Requested by: [email protected] (@hbjORbj)

devin-ai-integration bot and others added 3 commits November 21, 2025 13:07
…ervice

- Created WorkflowReminderRepository to handle workflow reminder queries
- Refactored cancelScheduledMessagesAndScheduleEmails to accept userIdsWithoutCredits parameter
- Moved credit-checking logic from workflows to CreditService
- Changed dynamic import to static import in CreditService
- Updated tests to pass userIdsWithoutCredits parameter
- Removed unused imports (prisma, WorkflowMethods)

This creates a one-way dependency (billing -> workflows) and follows the repository pattern by removing direct Prisma usage from reminderScheduler.

Co-Authored-By: [email protected] <[email protected]>
- Use MembershipRepository.listAcceptedTeamMemberIds instead of findAllAcceptedPublishedTeamMemberships
- Re-add prisma import to reminderScheduler.ts for UserRepository
- Update WorkflowReminderRepository to use WorkflowActions enum instead of string

Co-Authored-By: [email protected] <[email protected]>
…jection

Break the 4-file circular dependency chain:
credit-service → reminderScheduler → smsReminderManager → messageDispatcher → credit-service

Solution:
- Add optional creditCheckFn parameter to messageDispatcher functions
- Thread creditCheckFn through the call chain: scheduleWorkflowReminders → scheduleSMSReminder/scheduleWhatsappReminder → messageDispatcher
- When creditCheckFn is provided, use it; otherwise fall back to dynamic CreditService import for backward compatibility
- This breaks the workflows → billing import while preserving immediate fallback behavior

Changes:
- messageDispatcher: Accept optional creditCheckFn parameter, use it if provided
- smsReminderManager: Thread creditCheckFn through scheduleSMSReminder
- whatsappReminderManager: Thread creditCheckFn through scheduleWhatsappReminder
- reminderScheduler: Add creditCheckFn to ScheduleWorkflowRemindersArgs and pass through processWorkflowStep

All type checks, lint checks, and unit tests pass.

Co-Authored-By: [email protected] <[email protected]>
@devin-ai-integration
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@devin-ai-integration
Copy link
Contributor

Closing this PR in favor of #25343.

This PR accidentally included commits from PR #25312 (reminderScheduler fix) because I branched from the wrong base. The user explicitly requested the messageDispatcher fix to be in a separate PR from the reminderScheduler fix.

PR #25343 is a clean branch from main with only the messageDispatcher circular dependency fix.

@devin-ai-integration
Copy link
Contributor

Superseded by #25343

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

Labels

core area: core, team members only foundation size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants