feat: export txn history as csv#2210
Conversation
|
Claude finished @ethicnology's task in 6m 16s —— View job PR Review: CSV Transaction Export (v1)Todos:
Overall, the slice is well-structured — a port + adapter + dedicated use case + cubit + screen, with a unit test for the use case. It mostly respects AGENTS.md. A handful of issues are worth addressing before merge. 🔴 Bugs / correctness1. Date filter mixes local and UTC time — final inclusiveEnd = end == null
? null
: DateTime(end.year, end.month, end.day, 23, 59, 59);
The unit test Fix: normalise both ends to UTC explicitly. Either accept UTC dates from the picker ( 2. Status reports the wallet-tx status when a swap is in flight —
3. For a Lightning send/receive, 🟠 Architecture / AGENTS.md compliance4. This error describes an application-layer condition ("the filtered query returned nothing"), not a domain invariant. ARCHITECTURE.md rule #11 / AGENTS.md errors-per-layer rule expects:
The existing
5. Cubit leaks raw exception strings to the UI — } catch (e) {
emit(ExportTransactionsState.error(message: e.toString()));
}AGENTS.md rule #11: "Map foreign errors at every layer boundary; never leak another layer's error type." The UI renders 6. Filename construction lives in the cubit — Mild but worth noting per AGENTS.md rule #4 (BLoCs are thin). Pushing the filename + bytes encoding into the use case (returning a small Also: the filename embeds 7. Registration nit —
8. Single-file folders — AGENTS.md rule #14 says don't create the folder until a second file exists, unless it's "imminent in the same PR." The TODO at the top of 🟡 UX / polish9. Date display ignores locale — Hardcoded 10. Sort order is oldest-first, ascending — 11. Translations — every locale got the new English keys translated except the two ✅ What looks good
🧪 Test gapsThe use case test covers wallet-only transactions. Add cases for:
SummarySolid first cut. Top three things to address before merge:
The status-precedence (#2) and lightning-network labelling (#3) are worth a quick chat with whoever scoped #1363 — they're product decisions, not bugs. |
|
@basantagoswami any answers regarding Claude review ? |
|
@ethicnology comparing Claude with my codex bb-review skill
|
|
debug apk available here https://github.com/SatoshiPortal/bullbitcoin-mobile/actions/runs/26586256138 |
…in, and date filtering
review again |
|
Claude finished @basantagoswami's task in 6m 39s —— View job Re-review of v2 — most prior items addressed, two AGENTS.md issues remainTodos:
Solid pass on the previous feedback — every functional item from my v1 review and most of the Codex items are addressed (UTC/half-open boundary, swap-status precedence, LN network/address, saver port + filename, locale-aware date display, newest-first sort, reversed-range validation, screen overflow, scrollable body + pinned CTA, tests for LN/chain-swap/payjoin/null-timestamp). Two AGENTS.md compliance items still need attention before merge, plus a couple of smaller flags. 🔴 AGENTS.md compliance — please address1. Error file name violates rule #11 — AGENTS.md rule #11 (and ARCHITECTURE.md §"Error handling") is explicit:
The new file is both feature-prefixed (
2. New cross-feature edge import 'package:bb_mobile/features/transactions/ui/transactions_router.dart';This adds a previously-absent a) Bypasses facade convention (rule #1). Compare to the labels button in the same file: it does b) FEATURES.md graph not updated (rule #13). Rule #13 says any PR that changes which features another feature consumes must update the mermaid graph in the same commit. The new 🟠 Worth a discussion (not blockers)3. After the new dedup (commit Two clean options:
The current test (line 364) encodes today's behavior but doesn't argue why — and Codex's flag #2 still applies to it. 4. swapFeeSats: swap?.fees?.totalFees(swap.amountSat).toString() ?? '',
5. The use-case unit test instantiates the concrete Couples the use-case test to the CSV format. Fine while CSV is the only format, but if a second formatter ever lands (e.g. BIP-329 JSON), these tests will start asserting CSV-specific column indices for behavior that should be format-agnostic. Worth a ✅ What's resolved / good
SummaryTwo AGENTS.md items to address (error filename + facade/FEATURES.md edge); the chain-swap |
basantagoswami
changed the title
|
Replace presence checks (socket file exists, daemon process running, keyring file exists) with functional probes (bus accepts Peer.Ping, secret-tool store/clear round-trips). Presence checks passed when a crash left a stale dbus socket, or when the keyring file existed but was locked with a forgotten password, causing the app to die at init with "Failed to unlock the keyring". Also runs the bootstrap store/clear unconditionally (cheap and idempotent) and asserts usability at the end of postStartCommand so failures surface here instead of mid-app-init.
Presence checks (socket file exists, daemon process running, keyring file present) passed when a crash left a stale dbus socket or when the keyring file existed but was locked with a forgotten password, causing the app to die at init with libsecret_error: Failed to unlock the keyring. Replace with functional probes: dbus Peer.Ping, and secret-tool store/clear round-trip against the default collection. Bootstrap runs unconditionally (cheap, idempotent), and a final assertion fails postStartCommand here rather than mid-app-init.
Replace the inline QrImageView in ShowAnimatedQrWidget with the shared QrDisplayWidget so the QR keeps a theme-independent ~26px white quiet zone (16px surfaceFixed padding + qr_flutter's internal padding) and a contain-fit 300x300 size. Large PSBT payloads previously filled the parent Container — whose color is appColors.background (black in dark mode) — leaving the camera's auto-exposure unable to resolve the QR boundary on Jade. Fixes #1917.
|
rebased on develop |
…clude expired swaps
5 participants
Fixes #1363