Skip to content

fix(dive-computer): log download failures to the file log#258

Merged
ericgriffin merged 2 commits into
mainfrom
fix/dc-download-error-logging
Apr 22, 2026
Merged

fix(dive-computer): log download failures to the file log#258
ericgriffin merged 2 commits into
mainfrom
fix/dc-download-error-logging

Conversation

@ericgriffin
Copy link
Copy Markdown
Member

Summary

  • DownloadNotifier handled DownloadErrorEvent and caught startDownload exceptions by updating UI state only, never calling the logger. A user reporting "Download failed" on a Shearwater Teric over Windows 11 BLE had no [LDC] or [ERROR] entries in submersion.log, making remote diagnosis impossible.
  • Both error paths now emit _log.error(...) under LogCategory.libdc. The Dart catch block also forwards the exception and stack trace; the native DownloadErrorEvent handler logs the error code and message from libdivecomputer.

Why

Without a logged error, a libdc-reported BLE comms failure, a permission error on startDownload, and a "download completed with 0 dives" outcome are all indistinguishable in a user-submitted log. This blocks support triage on exactly the flows most likely to break (BLE dive computers on Windows).

Test plan

  • flutter test test/features/dive_computer/presentation/providers/download_notifier_fingerprint_test.dart — 5/5 pass (2 new)
  • New tests subscribe to LoggerService.logStream, assert exactly one LogLevel.error entry per failure path with the expected category and message substrings
  • Tests verified RED first (assertion hasLength(1) vs []), then GREEN after the fix
  • dart format — clean
  • flutter analyze — "No issues found"
  • Full pre-push suite — 7098 tests passing

The DownloadNotifier handled DownloadErrorEvent and caught startDownload
exceptions by updating UI state only, never calling the logger. Users
reporting "Download failed" had no [LDC] or [ERROR] entries in their log
file, making remote diagnosis impossible.

Now both error paths emit _log.error(...) under LogCategory.libdc with
the error code, message, and (for the Dart catch) stack trace.

Tested via two new cases in download_notifier_fingerprint_test.dart that
subscribe to LoggerService.logStream and assert exactly one ERROR entry
per failure path.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves diagnosability of dive-computer download failures by ensuring DownloadNotifier emits LogLevel.error entries under LogCategory.libdc for both plugin error events and Dart-side startDownload exceptions, and adds tests to verify those logs are produced.

Changes:

  • Add libdc error logging when startDownload throws (including exception + stack trace).
  • Add libdc error logging when a DownloadErrorEvent is received (including error code/message).
  • Add tests that subscribe to LoggerService.logStream and assert exactly one error log entry per failure path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
lib/features/dive_computer/presentation/providers/download_providers.dart Add _log.error(...) calls for startDownload exceptions and DownloadErrorEvent handling.
test/features/dive_computer/presentation/providers/download_notifier_fingerprint_test.dart Add tests validating error logs are emitted for both failure paths.
Comments suppressed due to low confidence (1)

lib/features/dive_computer/presentation/providers/download_providers.dart:177

  • In the startDownload exception path, the download events subscription has already been created (_service.downloadEvents.listen(...)) but is not cancelled when startDownload throws. This can leave a dangling listener that may continue to receive events and mutate state after an immediate start failure. Cancel and null out _downloadSubscription in the catch block (and consider also calling _service.cancelDownload() if appropriate) to keep state consistent and avoid leaks.
    } catch (e, stackTrace) {
      _log.error(
        'Download failed',
        category: LogCategory.libdc,
        error: e,
        stackTrace: stackTrace,
      );
      state = state.copyWith(
        phase: DownloadPhase.error,
        errorMessage: 'Download failed: $e',
      );
    }

Address PR review on #258:

- If _service.startDownload throws synchronously, the catch block now
  cancels _downloadSubscription so stray native events cannot mutate
  state (or double-log) after a start failure.
- Test cleanup moved to addTearDown so a failing expectation can no
  longer leak the LoggerService.logStream subscription into later tests.
- New tests filter the log stream by both LogLevel.error AND
  LogCategory.libdc to avoid false positives from unrelated error logs.
- Added a regression test that pushes a stray DownloadErrorEvent after a
  synchronous start failure and asserts only the catch-block log entry
  is captured.
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ericgriffin ericgriffin merged commit 2fe3df2 into main Apr 22, 2026
16 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Submersion Release Tracker Apr 22, 2026
@ericgriffin ericgriffin deleted the fix/dc-download-error-logging branch April 22, 2026 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants