Skip to content

refactor: use case から interfaces 層への逆依存を CronSchedulerPort で解消#187

Open
KinjiKawaguchi wants to merge 1 commit into
developfrom
refactor/invert-cron-scheduler-dependency
Open

refactor: use case から interfaces 層への逆依存を CronSchedulerPort で解消#187
KinjiKawaguchi wants to merge 1 commit into
developfrom
refactor/invert-cron-scheduler-dependency

Conversation

@KinjiKawaguchi

@KinjiKawaguchi KinjiKawaguchi commented Apr 7, 2026

Copy link
Copy Markdown
Member

Why

initializeScheduledMessagesFromConfig(use case)が interfaces/cron/scheduledMessageCron を直接 import しており、application → interfaces の逆依存が発生していた。

What

  • CronSchedulerPortapplication/ports/ に新設
  • ScheduledMessageCronManagerCronSchedulerPort を実装
  • use case は port 経由でジョブをスケジュールするように変更
  • 不要になった addScheduledMessageJob / removeScheduledMessageJob を削除

How

依存方向: application/usecases → application/ports ← interfaces/cron に修正。
composition root(main.ts / main.local.ts)で scheduledMessageCronManagercronSchedulerPort として注入。

🤖 Generated with Claude Code


Open with Devin

initializeScheduledMessagesFromConfig が interfaces/cron を直接 import していた。
CronSchedulerPort を導入し、依存方向を application → interfaces から application → port ← interfaces に修正。

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 2 potential issues.

Open in Devin Review

Comment thread src/main.ts
Comment on lines +47 to 50
cronSchedulerPort: scheduledMessageCronManager,
};

// Cron manager に依存オブジェクトを設定
scheduledMessageCronManager.setDeps(deps);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: Circular reference between AppDeps and ScheduledMessageCronManager is intentional but worth noting

The deps object contains cronSchedulerPort: scheduledMessageCronManager (src/main.ts:47), and then scheduledMessageCronManager.setDeps(deps) is called at src/main.ts:50, creating a circular object reference (scheduledMessageCronManager.deps.cronSchedulerPort === scheduledMessageCronManager). This is safe in JavaScript (no infinite loop or stack overflow risk since it's just object references), and the setDeps pattern was pre-existing before this PR. However, it does mean the CronSchedulerPort implementation is tightly coupled to the full AppDeps at runtime, which somewhat undermines the abstraction goal. A future improvement could be to inject only the specific dependencies the cron manager needs (e.g., just sendScheduledMessage or a narrower deps subset) rather than the full AppDeps.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

*/
export interface CronSchedulerPort {
scheduleJob(id: string, cronSchedule: string): void;
cancelJob(id: string): void;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: cancelJob is defined on the port but has no callers yet

The CronSchedulerPort interface defines cancelJob(id) at src/application/ports/cronSchedulerPort.ts:6, and the implementation exists at src/interfaces/cron/scheduledMessageCron.ts:25-27, but there are currently no callers of cancelJob through the port anywhere in the codebase. The old removeScheduledMessageJob function that was removed also had no callers (it was only exported, never imported). This is not a bug — the method exists for future use (e.g., a delete-scheduled-message use case) — but it means the cancelJob path is untested through the port abstraction.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant