Repository navigation
reverse logs in csv export - #546
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe CSV export in the Logs component now builds the CSV body from a reversed copy of the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
a32eae4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e56faaa5.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://nit-reverse-logs-csv-file.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
ghost
left a comment
There was a problem hiding this comment.
🐛 Bug: .reverse() is on the wrong array
src/screens/Settings/Logs.tsx:112 — The .reverse() is applied to the values within each row, not to the rows themselves. This reverses the column order of the data while the header stays unchanged, producing a malformed CSV.
Current (with this PR applied):
Header: "msg","time","level"
Row: "level_val","time_val","msg_val" ← columns are flipped
The CSV header is built from Object.keys(logs[0]) (line 105) which stays in msg, time, level order. But each row's Object.values(row) is now .reverse()'d, so the data columns no longer match the header.
Intent (from the PR description): reverse row order so newer logs appear first.
Fix — reverse the logs array, not each row's values:
- const csvBody = logs
+ const csvBody = [...logs].reverse()
.map((row) =>
Object.values(row)
.map((k) => `"${k}"`)
- .reverse()
.join(','),
)
.join('\n')Using [...logs].reverse() avoids mutating the React state array.
Not protocol-critical — this is a UI-only log export. No security or fund-safety concerns.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/screens/Settings/Logs.tsx (1)
108-115:⚠️ Potential issue | 🔴 Critical
.reverse()is on the wrong array — breaks CSV column alignment and doesn't reverse log order.
.reverse()is applied toObject.values(row), which flips the column order within each row (e.g.,level,time,msg) whilecsvHeader(Lines 104–107) remains in original key order (msg,time,level). This produces a CSV where headers and values are misaligned — a data-correctness bug for anyone parsing the export.Additionally, per the PR objective (newer logs at the top so Slack previews show recent entries), the reversal should apply to the outer log list, not to each row's values.
🐛 Proposed fix
- const csvBody = logs - .map((row) => - Object.values(row) - .map((k) => `"${k}"`) - .reverse() - .join(','), - ) - .join('\n') + const csvBody = [...logs] + .reverse() + .map((row) => + Object.values(row) + .map((k) => `"${k}"`) + .join(','), + ) + .join('\n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Settings/Logs.tsx` around lines 108 - 115, The CSV generation currently calls .reverse() on Object.values(row), which flips column order and misaligns values with csvHeader; move the reversal to the outer logs array instead: iterate over logs.reverse() (or create a reversed copy) when building csvBody so each row's Object.values(row) stays in the same order as csvHeader (keep the mapping that wraps values in quotes and joins with commas), and remove .reverse() from inside the per-row mapping to preserve column alignment while ensuring newer logs appear at the top.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/screens/Settings/Logs.tsx`:
- Around line 108-115: The CSV generation currently calls .reverse() on
Object.values(row), which flips column order and misaligns values with
csvHeader; move the reversal to the outer logs array instead: iterate over
logs.reverse() (or create a reversed copy) when building csvBody so each row's
Object.values(row) stays in the same order as csvHeader (keep the mapping that
wraps values in quotes and joins with commas), and remove .reverse() from inside
the per-row mapping to preserve column alignment while ensuring newer logs
appear at the top.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2a6138d-359e-4306-9fd2-ba24e61304bc
📒 Files selected for processing (1)
src/screens/Settings/Logs.tsx
Deploying wallet-bitcoin with
|
| Latest commit: |
a32eae4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://47872cc2.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://nit-reverse-logs-csv-file.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
a32eae4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://16322f74.arkade-wallet.pages.dev |
| Branch Preview URL: | https://nit-reverse-logs-csv-file.arkade-wallet.pages.dev |
|
ACK |
ghost
left a comment
There was a problem hiding this comment.
Previous issue resolved ✅
The two follow-up commits correctly:
- Move
.reverse()to operate on the rows array (not column values) — fixes the header/data mismatch - Use
[...logs].reverse()spread copy to avoid mutating React state in place
LGTM. Clean, minimal change. No protocol impact.
* reverse logs in csv export * fix * avoid mutating the React state array
* remove ionic/normalize and ion-button * rebbit fixes * feat(wallet): prefix fiat amounts with currency symbols (#535) Amounts previously rendered with a trailing ISO code (`100.00 USD`, `50.00 EUR`) now use the common Unicode currency symbol as a prefix: `$100.00`, `€50.00`, `£25.00`, `¥1,000`. Currencies without a widely-used single-char symbol (CHF, CNY) keep the trailing-code form, so CNY does not collide with JPY's `¥`. The new formatters live alongside the existing generic ones: - `prettyFiatAmount(amount, currency)` and `prettyFiatHide(value, currency)` in `src/lib/format.ts` take the `Fiats` enum as a typed parameter. - The generic `prettyAmount` / `prettyHide` are intentionally left untouched so asset tickers (e.g. a stablecoin named `USD`) that flow through those helpers keep their trailing-code format. - Decimals are derived from the currency via `fiatDecimalsFor` in `src/lib/fiat.ts` (JPY = 0, others = 2) — shared between `FiatContext` and `prettyFiatAmount` so call sites no longer need to pass `fiatDecimals()`. Touched displays: hero balance (`Balance.tsx`), transaction list rows (`TransactionsList.tsx`), amount picker (`Keyboard.tsx`), transaction details (`Details.tsx`), Send/Receive/Notes success screens, and Send form available caption. Amount input labels (`InputAmount.tsx`) also use the symbol when available. Side fix: `Details.tsx` previously called `prettyAmount` without passing `fiatDecimals()`, hardcoding 2 decimals. JPY now renders as `¥174` / `¥0` in the transaction details instead of `174.00 JPY` / `0.00 JPY`. Hero alignment: the eye-toggle button wrapper now omits an empty unit `<Text>` when the symbol is inlined into the balance, and the button sits inside a `alignItems: center` flex container next to the unit text — keeping the icon centered with the smaller ISO code in the CHF case and snug against the balance in the `$7.30` case. Tests: - New coverage for `prettyFiatAmount` and `prettyFiatHide` in `src/test/lib/format.test.ts` (symbol + trailing-code + JPY 0-decimals). - Existing `prettyAmount` / `prettyHide` tests unchanged — the generic helpers still behave the same. - UI assertions updated to match the new rendered text: `src/test/screens/wallet/index.test.tsx` → `$0.00` `src/test/e2e/init.test.ts` → `$0.00` `src/test/e2e/keyboard.test.ts` → symbol-prefix regex Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> Co-authored-by: João Bordalo <bordalix@users.noreply.github.com> * Upgrade ts-sdk 0.4.16 - boltz-swap 0.3.17 (#539) * reverse logs in csv export (#546) * reverse logs in csv export * fix * avoid mutating the React state array * Extra tests (#548) * close copy sheet after copy * lint * test swaps in receive * Prevent swap after receival (#547) * Prevent swap after receival * fix * more removals * increase viewport size in tests * more removals * more removals * fixes * fixes * fixes * fix * remove ion-refresher * Merge branch 'master' into remove_ion-button * fix swap page (#549) * fix swap page * lint * add tests * Upgrade ts-sdk 0.4.17 - boltz-swap 0.3.18 (#551) * fix(receive): add spacing between button row and share button (#552) The vertical gap between the Add amount/Copy buttons and the Share button was too tight. Changed FlexCol gap from 0 to 0.75rem. Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> * Revert "Upgrade ts-sdk 0.4.17 - boltz-swap 0.3.18 (#551)" (#553) This reverts commit 1752cc8. * Upgrade ts-sdk 0.4.17 - boltz-swap 0.3.18 (#551) * Revert "Upgrade ts-sdk 0.4.17 - boltz-swap 0.3.18 (#551)" (#553) This reverts commit 1752cc8. * Upgrade ts-sdk 0.4.18 - boltz-swap 0.3.19 (#554) * add git commit to chatwoot custom attributes (#563) * add lnurl tests (#564) * lint * add lnurl tests Co-authored-by: Copilot <copilot@github.com> --------- Co-authored-by: Copilot <copilot@github.com> * fix(ui): top-align tx history rows when assets are present (#537) * fix(ui): top-align tx history rows when assets are present When a transaction has assets, the right column (sats + fiat + asset info) is taller than the left column (icon + label + date). The outer FlexRow used alignItems 'center' by default, pushing the sats amount above the label text. Switch to alignItems 'start' so both columns begin at the same vertical position. Also adds explicit 'between' to the outer FlexRow and removes a dead alignItems on the non-flex wrapper div. * show only 2 coins in the rigth side of a tx in txs list (#550) --------- Co-authored-by: João Bordalo <bordalix@users.noreply.github.com> * final removal Co-authored-by: Copilot <copilot@github.com> * Upgrade ts-sdk 0.4.19 - boltz-swap 0.3.20 (#565) * lint * fix box-sizing Co-authored-by: Copilot <copilot@github.com> * .label styles Co-authored-by: Copilot <copilot@github.com> * fix branta lint error (#568) * works on refresher Co-authored-by: Copilot <copilot@github.com> * fix unit tests Co-authored-by: Copilot <copilot@github.com> * remove ionic/react * fix label alignment * DRY: AssetCard Co-authored-by: Copilot <copilot@github.com> * fix inputAmount --------- Co-authored-by: Sahil Chaturvedi <sahilchaturvedi@protonmail.com> Co-authored-by: Sahil Chaturvedi <sahilc0@users.noreply.github.com> Co-authored-by: pietro909 <2094604+pietro909@users.noreply.github.com> Co-authored-by: Copilot <copilot@github.com> Co-authored-by: Andrew Camilleri <evilkukka@gmail.com>
NIT: reverse logs in CSV export so newer logs appear on top.
Useful for when Slack shows a preview of the CSV file in support channel.
@tiero wdyt?
Summary by CodeRabbit