Skip to content

refactor: introduce TaskError typed error envelope (#660)#665

Merged
lklimek merged 4 commits into
v1.0-devfrom
refactor/typed-errors-660
Feb 26, 2026
Merged

refactor: introduce TaskError typed error envelope (#660)#665
lklimek merged 4 commits into
v1.0-devfrom
refactor/typed-errors-660

Conversation

@lklimek
Copy link
Copy Markdown
Contributor

@lklimek lklimek commented Feb 26, 2026

Issue being fixed or feature implemented

Refs #660 (Steps 1–4 of the migration strategy)

User Story

Imagine you are a developer working on Dash Evo Tool. When a backend task fails, the error is currently a plain String — you can't distinguish a network timeout from a corrupted database from a bad user input. Now, errors are typed via TaskError: the UI shows a clean user-facing message (Display), while developer mode reveals the full typed diagnostic (Debug). Adding a new error domain is just one #[from] variant away.

Details

Replace TaskResult::Error(String) with TaskResult::Error(TaskError) — a typed error envelope that preserves error origin while maintaining full backwards compatibility via From<String>.

What was done?

  • TaskError enum (src/backend_task/error.rs) with three tiers:
    • Generic(String) + From<String> — zero-breakage backwards compat for all existing code
    • Other(Box<dyn Error + Send + Sync>) — catch-all for typed errors without a dedicated variant
    • #[from] + #[error(transparent)] variants for SpvError, DashPayError, ConfigError, GroveSTARKError, WalletError
  • run_backend_task returns Result<_, TaskError> — all domain runners still return Result<_, String>, converted automatically via From<String>
  • app.rs handler uses Display for banner text, Debug for collapsible details (developer mode only)
  • connection_status.rs adapted to use .to_string().contains(...) for RPC failure detection
  • CLAUDE.md documents the TaskError pattern
  • Manual test scenarios at docs/ai-design/2026-02-26-typed-errors/manual-tests.md

How has this been tested?

  • cargo clippy --all-features --all-targets -- -D warnings — clean
  • cargo +nightly fmt --all — clean
  • Manual test scenarios document at docs/ai-design/2026-02-26-typed-errors/manual-tests.md

Breaking Changes

None — From<String> ensures all existing Result<T, String> code compiles unchanged.

Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved error handling system with better categorization and clearer, user-friendly error messages across the application
    • Enhanced error recovery and connection status detection
  • Documentation

    • Added comprehensive test specifications and implementation guidelines for error handling improvements

lklimek and others added 3 commits February 26, 2026 14:14
Replace `TaskResult::Error(String)` with `TaskResult::Error(TaskError)`.
TaskError provides typed error propagation with Display for user-facing
messages and Debug for technical details, while From<String> ensures
zero-breakage backwards compatibility with all existing code.

- Create TaskError enum with Generic(String) + #[from] variants for
  SpvError, DashPayError, ConfigError, GroveSTARKError, WalletError
- Wire TaskResult::Error(TaskError) through app.rs, connection_status.rs,
  and query_dpns_contested_resources.rs
- Change run_backend_task to return Result<_, TaskError>
- app.rs handler now shows Display text in banner + Debug in details
- Document TaskError pattern in CLAUDE.md
- Add manual test scenarios

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…arent errors

- Add Other(Box<dyn Error + Send + Sync>) catch-all variant to TaskError
- Show Debug details in error banner only in developer mode
- Use #[error(transparent)] for proper error chain propagation

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MessageBanner on v1.0-dev takes &str, not impl Display. Convert
TaskError to string before passing to set_global and with_details.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Feb 26, 2026

📝 Walkthrough

Walkthrough

This PR migrates the backend task error system from Result<T, String> to a typed TaskError enum using thiserror, enabling structured error handling with separate Display messages for users and Debug output for logs. Core infrastructure updated across TaskResult enum, backend task function signatures, app error handling, and integration points.

Changes

Cohort / File(s) Summary
Error Infrastructure
src/backend_task/error.rs
Introduces new TaskError enum with Generic(String), Spv, DashPay, Config, GroveStark, Wallet variants using thiserror; implements From<String> for backward compatibility; Display delegates to error message, Debug includes variant details.
Task System Refactoring
src/backend_task/mod.rs, src/app.rs
Updates run_backend_task, run_backend_tasks_sequential, and run_backend_tasks_concurrent signatures from Result<..., String> to Result<..., TaskError>; wraps task results in Ok(...) and propagates errors as TaskError; changes TaskResult::Error variant from String to TaskError; wires error handling to convert TaskError to string for banner display and include Debug details in developer mode.
Error Propagation Updates
src/backend_task/contested_names/query_dpns_contested_resources.rs, src/context/connection_status.rs
Applies .into() conversion at error sites to align with new TaskError type; updates connection status error matching logic to inspect error text via to_string().
Module Exports
src/database/mod.rs
Adds public re-export of WalletError from wallet module.
Documentation
CLAUDE.md, docs/ai-design/2026-02-26-typed-errors/manual-tests.md
Documents TaskError pattern and migration guidance; adds comprehensive manual test specification covering error banner display, details expansion, backward compatibility, connection detection, edge cases, and integration scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Errors typed with care and grace,
No more strings to clutter the place!
Display for users, Debug for logs—
Now our messages don't get lost in the fog.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'refactor: introduce TaskError typed error envelope (#660)' directly and precisely summarizes the main change: introducing a new typed error envelope called TaskError to replace string-based errors.
Linked Issues check ✅ Passed The PR implements all core requirements from #660 Steps 1-4: TaskError enum with Generic(String) + From, TaskResult::Error(TaskError), multiple domain variants (#[from] SpvError, DashPayError, ConfigError, GroveSTARKError, WalletError), app.rs Display/Debug handling, and comprehensive documentation.
Out of Scope Changes check ✅ Passed All changes are scoped to the typed error migration: TaskError enum, function signature updates, error propagation wiring, connection status adaptation, and documentation. No unrelated modifications are present.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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/typed-errors-660

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

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/backend_task/mod.rs (1)

284-318: ⚠️ Potential issue | 🟠 Major

Backend task signatures now diverge from the current repository error contract.

These signatures switch to TaskError, but the active backend-task guideline still mandates Result<T, String>. Please align the authoritative policy/tooling with this migration in the same change set, or keep this layer on String until that policy update is in place.

As per coding guidelines src/backend_task/**/*.rs: Backend task errors should use Result<T, String> type where string errors display directly to users.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend_task/mod.rs` around lines 284 - 318, The new signatures use
TaskError but repository policy requires Result<T, String>; change
run_backend_tasks_concurrent and run_backend_task (and the synchronous
run_backend_tasks) to return Vec<Result<BackendTaskSuccessResult, String>> and
Result<BackendTaskSuccessResult, String> respectively, update the async closure
in run_backend_tasks_concurrent to return Result<BackendTaskSuccessResult,
String>, and convert any TaskError values inside these functions to String
(e.g., map_err(|e| e.to_string()) or .to_string()) so the layer consistently
uses Result<..., String> (references: run_backend_tasks_concurrent,
run_backend_task, and the earlier run_backend_tasks loop).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/ai-design/2026-02-26-typed-errors/manual-tests.md`:
- Around line 146-151: Add a language identifier to the fenced code block
starting at the shown error example so markdownlint MD040 is satisfied: change
the opening fence from ``` to ```text (or another appropriate language like
```console), keeping the block contents and closing fence unchanged; this
targets the fenced block that begins with "[Error Banner - Red]" in
manual-tests.md.

---

Outside diff comments:
In `@src/backend_task/mod.rs`:
- Around line 284-318: The new signatures use TaskError but repository policy
requires Result<T, String>; change run_backend_tasks_concurrent and
run_backend_task (and the synchronous run_backend_tasks) to return
Vec<Result<BackendTaskSuccessResult, String>> and
Result<BackendTaskSuccessResult, String> respectively, update the async closure
in run_backend_tasks_concurrent to return Result<BackendTaskSuccessResult,
String>, and convert any TaskError values inside these functions to String
(e.g., map_err(|e| e.to_string()) or .to_string()) so the layer consistently
uses Result<..., String> (references: run_backend_tasks_concurrent,
run_backend_task, and the earlier run_backend_tasks loop).

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a78539 and 8040648.

📒 Files selected for processing (8)
  • CLAUDE.md
  • docs/ai-design/2026-02-26-typed-errors/manual-tests.md
  • src/app.rs
  • src/backend_task/contested_names/query_dpns_contested_resources.rs
  • src/backend_task/error.rs
  • src/backend_task/mod.rs
  • src/context/connection_status.rs
  • src/database/mod.rs

Comment on lines +146 to +151
```
[Error Banner - Red]
Message: "Failed to connect to RPC: connection timeout"
[▼ Details]
Details: RpcError { reason: "connection timeout", code: -1, ... }
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Add a language identifier to the fenced block at Line 146.

This currently trips markdownlint MD040 (fenced-code-language).

📝 Proposed fix
-```
+```text
 [Error Banner - Red]
 Message: "Failed to connect to RPC: connection timeout"
 [▼ Details]
   Details: RpcError { reason: "connection timeout", code: -1, ... }
</details>

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>

[warning] 146-146: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

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

In @docs/ai-design/2026-02-26-typed-errors/manual-tests.md around lines 146 -
151, Add a language identifier to the fenced code block starting at the shown
error example so markdownlint MD040 is satisfied: change the opening fence from
totext (or another appropriate language like ```console), keeping the
block contents and closing fence unchanged; this targets the fenced block that
begins with "[Error Banner - Red]" in manual-tests.md.


</details>

<!-- fingerprinting:phantom:medusa:hawk -->

<!-- This is an auto-generated comment by CodeRabbit -->

@lklimek lklimek added the human review This PR is ready for human review. label Feb 26, 2026
Copy link
Copy Markdown
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

looks fine; I generally don't think we should be committing files like docs/ai-design/2026-02-26-typed-errors/manual-tests.md

@lklimek lklimek enabled auto-merge (squash) February 26, 2026 15:18
@lklimek lklimek merged commit 7fa30ed into v1.0-dev Feb 26, 2026
4 checks passed
@lklimek lklimek deleted the refactor/typed-errors-660 branch February 26, 2026 15:26
thepastaclaw added a commit to thepastaclaw/dash-evo-tool that referenced this pull request Mar 19, 2026
Resolve merge conflicts from upstream v1.0-dev commits including:
- TaskError typed error envelope refactor (dashpay#660/dashpay#665)
- SPV client interface API changes (run() now takes command channel)
- SyncState::Initializing new variant
- FeeRate -> FeeLevel API change
- NetworkExt trait import for known_genesis_block_hash
- Various UI component updates (hint_text removal, ComponentStyles)

Update e2e test code and kittest module list to work with current API.
Keep both e2e feature flag additions and v1.0-dev improvements.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

human review This PR is ready for human review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants