refactor: complete phase-5 handler DI migration to AppContext (#639)

[mostronatorcoder]

Phase 5 of #639 — Migrate remaining handlers to AppContext entrypoints

This PR completes Phase 5 by moving all remaining action routes in app.rs to &AppContext-based handler entrypoints.

What changed

1) New *_with_ctx entrypoints for all remaining action handlers

Added thin DI wrappers in each module to preserve current behavior while routing through AppContext:

• add_invoice_action_with_ctx
• admin_add_solver_action_with_ctx
• admin_cancel_action_with_ctx
• admin_settle_action_with_ctx
• admin_take_dispute_action_with_ctx
• dispute_action_with_ctx
• fiat_sent_action_with_ctx
• last_trade_index_with_ctx
• order_action_with_ctx
• orders_action_with_ctx
• update_user_reputation_action_with_ctx
• release_action_with_ctx
• restore_session_action_with_ctx
• take_buy_action_with_ctx
• take_sell_action_with_ctx
• trade_pubkey_action_with_ctx

cancel_action_with_ctx already existed from Phase 2.

2) Dispatcher (src/app.rs) now routes all handled actions through DI entrypoints

handle_message_action now calls only *_with_ctx action functions for handled action variants.

3) Removed legacy migration parameter from dispatcher signature

handle_message_action no longer takes a pool argument. ctx is now the dependency source for routed handlers.

Notes

• This phase intentionally keeps legacy inner functions (*_action(..., pool, ...)) as compatibility wrappers inside modules.
• Cleanup/removal of wrappers and remaining legacy paths is tracked separately in cleanup: remove DI migration scaffolding after phase 5 (#639 follow-up) #656.

Validation

• cargo fmt ✅
• cargo clippy --all-targets --all-features -- -D warnings ✅
• cargo test ✅ (190 passing)

Part of #639
Related cleanup tracker: #656

Summary by CodeRabbit

• Refactor
  • Restructured internal message handling architecture to use context-based action handlers, improving code organization and reducing parameter propagation complexity across handler functions.

[grunch]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f2fcf7e1-b90d-4a6c-839b-220b2f250d37

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

This PR refactors the action handler dispatcher by removing the direct database pool parameter from handle_message_action and introducing context-aware wrapper functions (*_with_ctx) for all action handlers. Each wrapper extracts the pool from the provided AppContext and delegates to the original handler.

Changes

Cohort / File(s)	Summary
Dispatcher Refactoring src/app.rs	Removed pool: &Pool<Sqlite> parameter from handle_message_action signature; updated all action dispatch calls to invoke *_with_ctx variants (e.g., order_action_with_ctx, take_sell_action_with_ctx) and updated corresponding imports.
Context-Aware Action Wrappers src/app/add_invoice.rs, src/app/admin_add_solver.rs, src/app/admin_cancel.rs, src/app/admin_settle.rs, src/app/admin_take_dispute.rs, src/app/dispute.rs, src/app/fiat_sent.rs, src/app/last_trade_index.rs, src/app/order.rs, src/app/orders.rs, src/app/rate_user.rs, src/app/release.rs, src/app/restore_session.rs, src/app/take_buy.rs, src/app/take_sell.rs, src/app/trade_pubkey.rs	Added new public async wrapper functions (*_with_ctx) that accept &AppContext, extract the pool via ctx.pool(), and delegate to existing action handlers without changing core logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

• cleanup: remove DI migration scaffolding after phase 5 (#639 follow-up) #656: Directly addresses the dispatcher and entrypoint cleanup objective by removing pool parameter from handle_message_action and migrating to context-based action handler routing.

Possibly related PRs

• refactor: add AppContext struct for dependency injection (#639 phase 1) #651: Introduces the AppContext abstraction that this PR builds upon by adding context-aware wrapper functions and updating the main dispatcher to use them.
• refactor: wire AppContext into dispatcher and migrate cancel module (#639 phase 2) #652: Implements the same AppContext-based migration pattern for the dispatcher and action handler calls.
• feat: improved in a more rusty way the connectionto db pool #487: Performs a related refactor of how the database pool is accessed and propagated through the codebase, eliminating direct pool parameter passing.

Suggested reviewers

• arkanoider
• Catrya

---

🐰 Wrappers dancing 'round the pool,
Context flows through like a rule,
No more passing pools about,
_with_ctx takes the shout! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately describes the main change: completing phase-5 of a handler dependency injection migration to AppContext, with reference to the related issue.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/phase5-migrate-remaining-handlers-639

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/app.rs (1)

250-285: Please extend the routing smoke test for these new branches.

handle_message_action_tests::test_action_routing_logic still omits Action::LastTradeIndex, Action::RestoreSession, and Action::Orders, so the routes added here can regress without a failing test.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/app.rs` around lines 250 - 285, The routing test
handle_message_action_tests::test_action_routing_logic is missing coverage for
Action::LastTradeIndex, Action::RestoreSession, and Action::Orders; update that
test to include cases for these three variants and assert they route to the
corresponding handlers (last_trade_index_with_ctx,
restore_session_action_with_ctx, orders_action_with_ctx) by constructing
messages/events that select those Actions and verifying the dispatched
path/handler is invoked or returned as expected so future changes to Action
routing will fail the test if regressions occur.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/app.rs`:
- Around line 246-250: The match arm for Action::PayInvoice currently panics via
todo!(); replace it with returning a typed Mostro error instead (e.g., construct
and return MostroError::MostroCantDo with an appropriate CantDoReason) so the
message loop handles unsupported actions gracefully; ensure the arm mirrors the
other arms' error handling by returning the error converted into the function's
Result/Error type (similar to how
add_invoice_action_with_ctx(...).await.map_err(|e| e.into()) is handled), and
reference the Action::PayInvoice match arm to implement this change.

---

Nitpick comments:
In `@src/app.rs`:
- Around line 250-285: The routing test
handle_message_action_tests::test_action_routing_logic is missing coverage for
Action::LastTradeIndex, Action::RestoreSession, and Action::Orders; update that
test to include cases for these three variants and assert they route to the
corresponding handlers (last_trade_index_with_ctx,
restore_session_action_with_ctx, orders_action_with_ctx) by constructing
messages/events that select those Actions and verifying the dispatched
path/handler is invoked or returned as expected so future changes to Action
routing will fail the test if regressions occur.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4b7907f7-ab54-41e0-b87b-05d3039a4c1b

📥 Commits

Reviewing files that changed from the base of the PR and between 280a809 and 4ab629e.

📒 Files selected for processing (17)

• src/app.rs
• src/app/add_invoice.rs
• src/app/admin_add_solver.rs
• src/app/admin_cancel.rs
• src/app/admin_settle.rs
• src/app/admin_take_dispute.rs
• src/app/dispute.rs
• src/app/fiat_sent.rs
• src/app/last_trade_index.rs
• src/app/order.rs
• src/app/orders.rs
• src/app/rate_user.rs
• src/app/release.rs
• src/app/restore_session.rs
• src/app/take_buy.rs
• src/app/take_sell.rs
• src/app/trade_pubkey.rs

[mostronatorcoder]

Follow-up hardening done in ee6b6d3 to make routing coverage assertive (not just static variant listing):

• Split dispatcher into:
  • handle_message_action_no_ln(...) for actions that don't require an LN connector
  • handle_message_action(...) for LN-requiring actions + delegation
• Added explicit async routing assertions for:
  • LastTradeIndex (expects CantDoReason::NotFound on empty test DB)
  • RestoreSession (expects Ok(()) for valid synthetic keys)
  • Orders (expects CantDoReason::InvalidParameters when payload is missing)
  • PayInvoice (expects typed CantDoReason::InvalidAction)

Validation:

• cargo fmt
• cargo clippy --all-targets --all-features -- -D warnings
• cargo test (193 passing)

[mostronatorcoder]

I agree with this review assessment.

• On PayInvoice behavior: this change is intentional and safety-motivated (reachable todo!() panic replaced by typed MostroCantDo(InvalidAction)). I documented this explicitly in PR discussion and kept it covered by a routing test.

• On routing-test coupling: fixed in b04e7b3.

  • LastTradeIndex and Orders routing tests now assert only error propagation (is_err()) instead of specific handler business errors.
  • Added inline comments clarifying the tests are dispatcher-path assertions, not handler-logic assertions.

Validation after changes:

• cargo fmt
• cargo clippy --all-targets --all-features -- -D warnings
• cargo test (193 passing)

[grunch]

LGTM