refactor: wire AppContext into dispatcher and migrate cancel module (#639 phase 2)

[mostronatorcoder]

Phase 2 of #639 — Wire AppContext into event loop + migrate cancel.rs

Depends on #651 (Phase 1 — AppContext struct). This PR includes Phase 1 commits.

Changes

src/app.rs (dispatcher)

• Build AppContext::from_globals() once per loop iteration in run()
• Pass &ctx to handle_message_action()
• Cancel action now uses ctx.pool() instead of the raw pool parameter

src/app/cancel.rs

• Added cancel_action_with_ctx(&AppContext, ...) — new DI-based entry point
• Existing cancel_action(pool, ...) preserved as legacy wrapper (no breaking changes)
• Internal helper functions (cancel_cooperative_execution_step_*, cancel_order_by_*, etc.) still accept &Pool<Sqlite> directly — they get it from the caller

Migration pattern demonstrated

This PR establishes the pattern for all remaining modules:

1. Add *_with_ctx variant that takes &AppContext
2. Keep legacy function as thin wrapper
3. Update dispatcher to pass ctx
4. Internal helpers receive pool from the _with_ctx caller

Once all modules are migrated, the legacy wrappers and pool parameter in handle_message_action can be removed.

What comes next

• Phase 3: Add mock implementations for AppContext test utilities
• Phase 4: Write unit tests for cancel.rs using mock contexts
• Phase 5: Migrate remaining modules following the same pattern

Validation

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

Part of #639

Summary by CodeRabbit

• Refactor
  • Introduced dependency-injection based application context for action handlers and runtime, enabling handlers to access shared services via context.
  • Routed cancellation and other public actions through context-aware entry points while keeping legacy routes for migration.
  • Simplified context internals (client ownership) and updated test utilities to reflect the new construction semantics.

[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]

Caution

Review failed

Pull request was closed or merged during review

Walkthrough

Refactors handlers to use dependency-injected AppContext: handle_message_action now accepts &AppContext, a context-aware cancel_action_with_ctx wrapper was added, and AppContext internals changed to hold a direct Client (removed Arc<Client>). The main run loop constructs and forwards AppContext.

Changes

Cohort / File(s)	Summary
Application entry src/app.rs	Constructs an AppContext from globals and forwards &AppContext into handle_message_action; updated handler signature to accept ctx: &AppContext and migrated cancel call sites to context-aware variants.
Cancel handler wrapper src/app/cancel.rs	Added cancel_action_with_ctx(ctx: &AppContext, ...) which delegates to existing cancel_action using ctx.pool(); imported AppContext and documented migration path while leaving legacy cancel_action intact.
Context internals & tests src/app/context.rs	Changed AppContext to store nostr_client: Client (removed Arc<Client>), updated constructors (new, from_globals) and test utilities (TestContextBuilder, with_nostr_client, defaults) to use Client directly.

Sequence Diagram(s)

sequenceDiagram
  rect rgba(200,220,255,0.5)
    participant Run as RunLoop
    participant Ctx as AppContext
    participant Handler as handle_message_action
    participant Cancel as cancel_action_with_ctx
    participant DB as DatabasePool
    participant LN as LndConnector
  end

  Run->>+Ctx: build AppContext(from_globals)
  Run->>+Handler: handle_message_action(msg, ctx)
  Handler->>+Cancel: cancel_action_with_ctx(ctx, msg, event, keys, ln_client)
  Cancel->>+DB: ctx.pool() -> execute cancel flow
  Cancel->>+LN: use ln_client for lightning ops
  Cancel-->>-Handler: Result
  Handler-->>-Run: Result propagation

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Dispute readability #390: Related refactor of handle_message_action and cancel flows touching the same call paths.
• refactor: add AppContext struct for dependency injection (#639 phase 1) #651: Prior PR introducing or modifying AppContext structure and its usage.

Suggested reviewers

• Catrya

Poem

"I hop with a context held tight,
No more globals in moonlight,
I pass my pouch to handlers anew,
DB, Nostr, Lightning — all in view,
A rabbit's patchwork, tidy and bright." 🐇✨

🚥 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 accurately summarizes the main changes: refactoring to integrate AppContext into the dispatcher and migrating the cancel module as part of Phase 2 of issue #639.
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 docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/migrate-cancel-to-context-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)

33-35: Route Cancel through cancel_action_with_ctx().

The cancel module already exposes the DI entry point, but the dispatcher still unwraps ctx.pool() itself and calls the legacy signature. Using the wrapper here keeps the context-to-pool translation inside src/app/cancel.rs and makes the migration pattern consistent.

♻️ Suggested cleanup

-use crate::app::cancel::cancel_action;
+use crate::app::cancel::cancel_action_with_ctx;
@@
-        Action::Cancel => cancel_action(msg, event, my_keys, ctx.pool(), ln_client)
+        Action::Cancel => cancel_action_with_ctx(ctx, msg, event, my_keys, ln_client)
             .await
             .map_err(|e| e.into()),

Also applies to: 262-263

🤖 Prompt for AI Agents

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

In `@src/app.rs` around lines 33 - 35, The dispatcher in app.rs currently calls
the legacy cancel_action by unwrapping ctx.pool() and passing the pool directly;
replace that call with cancel_action_with_ctx(ctx) so the context-to-pool
translation stays inside the cancel module (use the DI entry point
cancel_action_with_ctx instead of calling cancel_action(ctx.pool().unwrap(),
...)), and do the same substitution for the other occurrence referenced (lines
~262-263) to keep migration consistent.

🤖 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/context.rs`:
- Around line 19-33: The example docs currently call cancel_action(ctx, ...) but
the exported DI entry point is cancel_action_with_ctx and the original
cancel_action still expects &Pool<Sqlite>; update the example to call
cancel_action_with_ctx and adjust the parameters to match its signature (use
AppContext-derived ctx usage as in other examples), e.g., replace
cancel_action(...) with cancel_action_with_ctx(ctx, msg, event, my_keys,
ln_client) and ensure the example imports/uses AppContext and obtains ctx via
the same DI pattern used elsewhere in the file (refer to AppContext,
cancel_action_with_ctx, and pool()/settings() usages to align the example).

---

Nitpick comments:
In `@src/app.rs`:
- Around line 33-35: The dispatcher in app.rs currently calls the legacy
cancel_action by unwrapping ctx.pool() and passing the pool directly; replace
that call with cancel_action_with_ctx(ctx) so the context-to-pool translation
stays inside the cancel module (use the DI entry point cancel_action_with_ctx
instead of calling cancel_action(ctx.pool().unwrap(), ...)), and do the same
substitution for the other occurrence referenced (lines ~262-263) to keep
migration consistent.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d4b7ee59-3f7d-444c-a1ec-bb4ae04e07f1

📥 Commits

Reviewing files that changed from the base of the PR and between f071baa and 4f27ac3.

📒 Files selected for processing (3)

• src/app.rs
• src/app/cancel.rs
• src/app/context.rs

[grunch]

Three issues to fix before merge:

1. Double-wrapping the Nostr client

let nostr_client = Arc::new(get_nostr_client()?.clone());

Client in nostr-sdk is already an Arc-backed handle internally — cloning it gives a second handle sharing the same relay pool. Wrapping that clone in another Arc is redundant and will cause Arc::try_unwrap() to fail unexpectedly for anyone expecting exclusive ownership in future phases. Either store the cloned Client directly, or if you need Arc, clone the outer Arc from the global rather than creating a new one.

2. ctx is a snapshot — document it

from_globals() is called once before the while loop, capturing Client and Settings at that moment. If the Nostr client reconnects or settings are reloaded at runtime, ctx will hold stale references. The pool is fine (it's an Arc to the same pool). Add an explicit comment:

// NOTE: ctx captures Client and Settings at startup. If either is
// reloaded at runtime, rebuild ctx or access them via globals directly.
let ctx = AppContext::from_globals()
    .expect("Failed to build AppContext — globals not initialized");

Without this, whoever maintains this code will assume ctx is always fresh.

3. handle_message_action has two sources of truth for the pool

async fn handle_message_action(
    ...
    pool: &Pool<Sqlite>,   // used by 18 handlers
    ln_client: &mut LndConnector,
    ctx: &AppContext,      // ctx.pool() is the same pool
)

Both pool and ctx.pool() point to the same SqlitePool. This is expected during migration, but without a comment it looks like a mistake. Add:

// pool: legacy parameter kept during migration; new handlers use ctx.pool() instead.
// Remove once all handlers are migrated (issue #639).

[mostronatorcoder]

Good review — I verified each point against current code and applied only what was needed in 1667a2b:

1. Nostr client double-Arc concern

   • AppContext now stores nostr_client: Client (not Arc<Client>).
   • from_globals() now uses get_nostr_client()?.clone() directly (no extra Arc::new(...)).

2. ctx snapshot behavior

   • Added explicit note in run() near AppContext::from_globals() that ctx captures Client/Settings at construction and must be rebuilt if reloaded.

3. Two pool sources during migration

   • Added migration comments in handle_message_action clarifying pool is legacy and should be removed after all handlers migrate to ctx.pool() (issue refactor: introduce dependency injection for testability #639).

Validation:

• cargo fmt
• cargo clippy --all-targets --all-features -- -D warnings
• cargo test

[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.

[grunch]

tACK