Skip to content

refactor(address_view): sanitize user-facing error messages - #2543

Merged
ethicnology merged 3 commits into
developfrom
refactor-address-view
Aug 3, 2026
Merged

refactor(address_view): sanitize user-facing error messages#2543
ethicnology merged 3 commits into
developfrom
refactor-address-view

Conversation

@wired-pasteque

Copy link
Copy Markdown
Collaborator

No description provided.

@wired-pasteque wired-pasteque self-assigned this Jul 30, 2026
@wired-pasteque
wired-pasteque force-pushed the refactor-address-view branch from 4c70dd4 to 5d8c14d Compare July 30, 2026 14:17
@wired-pasteque

This comment was marked as outdated.

@claude

This comment was marked as outdated.

@wired-pasteque

Copy link
Copy Markdown
Collaborator Author

@claude, check the fixes

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Claude finished @wired-pasteque's task in 1m 42s —— View job


Fix check for b98f9af02 — all applied fixes verified correct ✅

  • Review fix commit b98f9af02 against the previous audit findings
  • Verify each finding (logging signal, spinner-next-to-error, test updates)
  • Post results

The commit addresses the two actionable findings from the previous audit (#1 logging signal loss, #3 spinner next to error row), and I traced both fixes end-to-end — they're correct, and the privacy property the PR is built around still holds. Analyze and Test CI is green on this commit (run).

Finding #1 — logging signal loss: fixed, and the privacy claim checks out

Both usecases now pass the raw exception to the logger (get_address_list_usecase.dart:42, check_wallet_is_liquid_usecase.dart:22) instead of e.runtimeType, restoring the diagnostic signal that was flagged. The updated test doc-comments claim this is safe because Sentry's beforeSend strips the message — I verified the full path rather than taking the comment's word for it:

  • log.severe routes to Report.errorSentry.captureException(exception, ...) (lib/core/utils/report.dart:315), and beforeSend nulls every exception value before anything leaves the device: event.exceptions?.forEach((e) => e.value = null) (lib/core/utils/report.dart:178). So Sentry gets exception type + stack trace (the signal that was missing), never the message that could embed a wallet id.
  • log.warning (the liquid-check path) doesn't route to Sentry at all — local log only.
  • The sanitization boundary the tests pin is unchanged: Failure.logMessage still carries only e.runtimeType.toString(), and the sentinel-wallet-id assertions still guard it.

One deliberate consequence, which the updated test comments state explicitly: the raw exception message (potentially containing the wallet id) now lands in the local on-disk log, which users can export and hand to support. That's the user's own data and a documented trade-off, so I consider it fine — just making sure it's a visible decision, not an accident.

Finding #3 — perpetual spinner below the error row: fixed

addresses_screen.dart:147 now computes itemCount as addresses.length + (hasReachedEnd || failure != null ? 0 : 1) + (failure != null ? 1 : 0). I walked the three states against the itemBuilder:

  • pagination failure → exactly one extra row, and since failure != null && index == addresses.length, it renders the error text — no spinner;
  • loading more (no failure, not at end) → one extra row → spinner, as before;
  • reached end → no extra rows.

Findings #2 and #4 — intentionally not addressed (fine)

The silently-kept isLiquid on a failed wallet-kind check (#2) and the .arb key placement (#4) were flagged as non-blocking observations, not defects, and this commit doesn't touch the bloc or the localization files. No objection — #2 matches pre-refactor behavior and #4 is cosmetic.

Limitations

No Flutter SDK on this runner, so I couldn't run the suite locally — but the green Analyze and Test run on this exact commit covers analyzer + tests, including the updated sentinel-id tests.

@wired-pasteque
wired-pasteque marked this pull request as ready for review August 3, 2026 12:30
@ethicnology
ethicnology merged commit ec1e5b1 into develop Aug 3, 2026
2 checks passed
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.

2 participants