log on initial load, swap layout fix, spendable balance fix#77
Merged
Conversation
📝 WalkthroughWalkthroughAdded balance normalization and UTXO-derived spend categorization in the taker API; replaced fixed swap circuit SVGs with adaptive layouts and responsive sizing; extended swap report/history normalization and markup helpers; added runtime sync/log UI and wallet debug logging; adjusted Tailwind-derived CSS utilities and theme tokens. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client(UI)
participant API as TakerAPI (`taker:getBalance`)
participant UTXO as UTXOService (`listAllUtxoSpendInfo`)
participant FS as FileSystem (`DATA_DIR/swap_reports`)
UI->>API: request getBalance()
API->>UTXO: listAllUtxoSpendInfo()
UTXO-->>API: utxoSpendInfo
API->>FS: getAllSwapReportPaths() / read swap_reports
FS-->>API: swap report files / historical mappings
API->>API: deriveBalancesFromUtxos(utxoSpendInfo, historicalMap)
API->>API: normalizeBalancePayload(rawBalances, derivedTotals)
API-->>UI: { balance: normalized, debug }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api1.js`:
- Around line 247-252: The recalculation of normalized.regular after applying
historicalSwapBalance is incorrect because it only subtracts normalized.swap
from normalized.spendable and ignores normalized.contract and
normalized.fidelity; update the logic in the block that compares
historicalSwapBalance and normalized.swap (where normalized.swap,
normalized.regular, normalized.spendable are adjusted) to set normalized.regular
= Math.max(0, normalized.spendable - normalized.swap - normalized.contract -
normalized.fidelity) so regular reflects all occupied categories and stays
non-negative.
In `@src/components/swap/Coinswap.js`:
- Around line 1146-1149: The anchor markup for arrow links currently includes an
inline onclick that prevents navigation; remove the
onclick="event.preventDefault();" from the template returned in the rendering
code that creates `<a id="arrow-link-${i}" ...>` so clicks are no longer
blocked, and ensure the anchor opens externally by adding target="_blank" and
rel="noopener noreferrer" (or alternatively have updateArrowLink() attach a
click handler that calls window.open(url, '_blank')). Keep the element id
pattern `arrow-link-${i}` and the `updateArrowLink()` logic intact so injected
tx URLs are navigable in a new tab.
- Around line 942-953: Normalize and validate swapData.makers before using it to
compute actualMakers/totalNodes: coerce swapData.makers to an integer (e.g., via
parseInt or Number) and handle NaN by falling back to a safe default, then clamp
the value to a sensible range (minimum 1 to avoid degenerate geometry and an
optional upper bound) so that totalNodes = actualMakers + 1 computes numerically
rather than string-concatenating; update the logic that sets actualMakers (and
any downstream size computations that use youHalf, makerHalf, youFont,
makerFont, youRx, makerRx, maxNodeHalf, labelPad) to use this validated numeric
value.
In `@src/components/swap/SwapHistory.js`:
- Around line 149-223: SwapHistoryComponent currently duplicates the
markup/formatting logic that already exists in the exported
buildSwapHistoryMarkup/buildSwapHistoryList and helper functions (satsToBtc,
formatRelativeTime, formatDate, formatDuration); update SwapHistoryComponent to
call those exported renderers/helpers instead of reimplementing them, remove the
duplicated formatting code inside SwapHistoryComponent (lines with local
satsToBtc/formatRelativeTime/formatDate/formatDuration implementations and the
inline mapping that mirrors buildSwapHistoryMarkup), and import/use
buildSwapHistoryMarkup or buildSwapHistoryList to produce the HTML (ensuring any
data transformations are done once via the shared helpers).
- Around line 136-146: summarizeSwapHistory and satsToBtc assume numeric inputs
and buildSwapHistoryMarkup renders swap.amount, swap.totalOutputAmount and
swap.feePercentage without coercion; coerce those fields to numbers (e.g., use
Number(...) or parseFloat(...) with a fallback of 0) before using reduce,
toFixed, toLocaleString or any arithmetic so string values don’t cause
concatenation or NaN; update summarizeSwapHistory to coerce s.amount and
s.totalFee inside the reducers, update satsToBtc to accept numeric strings by
converting sats to Number and handling NaN, and update buildSwapHistoryMarkup to
coerce swap.amount, swap.totalOutputAmount and swap.feePercentage before
formatting/displaying.
- Around line 133-134: The function now returns stale swapHistory when getAll()
fails; update the fetch error handling inside the function that calls getAll()
(the one that currently ends with "return swapHistory;") so that on any fetch
failure you clear swapHistory (set to [] or empty the array) and then rethrow
the error (or return []), instead of swallowing it; reference the swapHistory
variable and the getAll() call so callers (e.g., the component expecting errors)
receive the failure and can show the new error UI.
In `@src/components/swap/SwapReport.js`:
- Around line 491-643: Duplicate adaptive layout code (getRectPoint and
buildAdaptiveLayout) should be extracted into a shared helper and used by both
SwapReport.js and Coinswap.js; create a new utility (e.g., export function
buildAdaptiveLayout(params) and export getRectPoint) that accepts the external
dependencies currently referenced (actualMakers, totalNodes, maxNodeHalf,
labelPad and any gap or constants) and returns the same object shape (centerX,
centerY, svgWidth, svgHeight, positions, guideMarkup), replace the local
buildAdaptiveLayout/getRectPoint implementations in both files with imports from
the new helper and update call sites to pass the required params so spacing,
arrow padding and overflow logic stay consistent across both components.
- Around line 478-489: report.makersCount may be a string so actualMakers
becomes incorrect (e.g., '3' leads to '31'); convert and validate it before
doing layout math: coerce report.makersCount to a number (e.g., Number or
parseInt) with a safe fallback to 0 if NaN, then use actualMakers = Math.max(0,
coercedValue || report.makerAddresses.length) and compute totalNodes from that
numeric actualMakers; update uses in layout variables (youHalf, makerHalf,
youFont, makerFont, youRx, makerRx, maxNodeHalf, labelPad) so downstream
geometry never gets NaN or malformed sizes for zero or string inputs.
In `@src/js/app.js`:
- Around line 445-447: Update the error logging in the catch block that
currently logs "Failed to refresh launch sync logs" so it follows the project
convention; replace the existing console.error call in that catch (the one
referencing 'Failed to refresh launch sync logs') with the standardized format
console.error('⚠️ Failed to refresh launch sync logs:', error) so the message
text is preserved but uses the required '⚠️ Context:' style.
- Around line 509-512: The final refresh call can throw and block completion;
wrap the await refreshLaunchLogs() in its own try-catch so any error is caught
and logged but not rethrown. Specifically, inside the finally block that
references logPoll and calls refreshLaunchLogs(), change it to
clearInterval(logPoll) as before and then try { await refreshLaunchLogs(); }
catch (err) { /* log error via console.error or existing logger */ } so failures
in refreshLaunchLogs() are non-fatal and do not prevent onComplete() or
subsequent cleanup from running.
- Line 495: The empty catch is swallowing errors during the sync-status polling
loop; modify the catch to accept an error parameter and log it before clearing
the interval and resolving (e.g., change "catch { clearInterval(poll);
resolve(); }" to "catch (err) { console.error('Sync polling error', err);
clearInterval(poll); resolve(); }" or use the existing app logger), referencing
the "poll" interval variable and the "resolve" callback so the behavior remains
the same after logging.
- Around line 476-485: The progress-bar width calculation should explicitly
handle the 'starting' syncStatus instead of falling through to the default 76%;
update the conditional around sync.progress and syncStatus (the code that
computes nextWidth and sets progressBar.style.width) to include a branch like
syncStatus === 'starting' ? 18 (or another appropriate initial value) so that
when sync.progress is not a number and status is 'starting' the bar shows the
intended initial width; keep the existing bounds/clamping (Math.max/Math.min)
for the numeric progress branch and ensure the order of checks preserves numeric
progress > 'starting' handling.
- Around line 343-347: The parsed log object currently sets timestamp: new
Date(match[1]).getTime() which can be NaN for malformed date strings; update the
parsing in the function that returns this object (the block using match[1],
match[2], match[3]) to validate the date and provide a fallback (e.g., use
Date.now() or 0) when isNaN(timestamp) is true so the timestamp field is always
a number and won't be filtered out by the syncStartedAt comparison elsewhere;
ensure you still lowercase match[2] and preserve match[3] unchanged.
In `@src/styles/output.css`:
- Around line 1267-1269: The generated CSS contains formatting that fails
stylelint's declaration-empty-line-before rule around the .tracking-\[0\.2em\]
rule (and the nearby block around the same tracking utility), so regenerate or
reformat the compiled output: either re-run the source stylesheet build
(Tailwind/PostCSS) to produce a properly formatted output or run stylelint --fix
/ apply the project's CSS formatter to remove or add the required empty line
before declarations to satisfy declaration-empty-line-before for the
.tracking-\[0\.2em\] rule and the corresponding block referenced in the diff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b01eea93-93b8-4a7f-821e-2297b24d52ac
📒 Files selected for processing (7)
api1.jssrc/components/swap/Coinswap.jssrc/components/swap/SwapHistory.jssrc/components/swap/SwapReport.jssrc/components/wallet/Wallet.jssrc/js/app.jssrc/styles/output.css
Comment on lines
+445
to
+447
| } catch (error) { | ||
| console.error('Failed to refresh launch sync logs:', error); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Error logging could follow the project convention.
The coding guidelines specify using console.error('⚠️ Context:', error) format for error logging.
✏️ Suggested change
} catch (error) {
- console.error('Failed to refresh launch sync logs:', error);
+ console.error('⚠️ Failed to refresh launch sync logs:', error);
}As per coding guidelines: "Log errors with context using console.error('
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/app.js` around lines 445 - 447, Update the error logging in the catch
block that currently logs "Failed to refresh launch sync logs" so it follows the
project convention; replace the existing console.error call in that catch (the
one referencing 'Failed to refresh launch sync logs') with the standardized
format console.error('⚠️ Failed to refresh launch sync logs:', error) so the
message text is preserved but uses the required '⚠️ Context:' style.
Comment on lines
+476
to
+485
| const nextWidth = | ||
| typeof sync.progress === 'number' | ||
| ? Math.max(18, Math.min(100, sync.progress)) | ||
| : syncStatus === 'completed' | ||
| ? 100 | ||
| : syncStatus === 'failed' | ||
| ? 100 | ||
| : 76; | ||
| progressBar.style.width = `${nextWidth}%`; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider handling 'starting' status explicitly for consistency.
Per Market.js:168-172, the sync status can also be 'starting' in addition to 'syncing', 'completed', and 'failed'. The current fallback logic works but the implicit handling may cause confusion when debugging why the progress bar shows 76% during initialization.
✏️ Optional: explicit handling
if (progressBar) {
const nextWidth =
typeof sync.progress === 'number'
? Math.max(18, Math.min(100, sync.progress))
: syncStatus === 'completed'
? 100
: syncStatus === 'failed'
? 100
- : 76;
+ : syncStatus === 'starting'
+ ? 25
+ : 76;
progressBar.style.width = `${nextWidth}%`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const nextWidth = | |
| typeof sync.progress === 'number' | |
| ? Math.max(18, Math.min(100, sync.progress)) | |
| : syncStatus === 'completed' | |
| ? 100 | |
| : syncStatus === 'failed' | |
| ? 100 | |
| : 76; | |
| progressBar.style.width = `${nextWidth}%`; | |
| } | |
| const nextWidth = | |
| typeof sync.progress === 'number' | |
| ? Math.max(18, Math.min(100, sync.progress)) | |
| : syncStatus === 'completed' | |
| ? 100 | |
| : syncStatus === 'failed' | |
| ? 100 | |
| : syncStatus === 'starting' | |
| ? 25 | |
| : 76; | |
| progressBar.style.width = `${nextWidth}%`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/app.js` around lines 476 - 485, The progress-bar width calculation
should explicitly handle the 'starting' syncStatus instead of falling through to
the default 76%; update the conditional around sync.progress and syncStatus (the
code that computes nextWidth and sets progressBar.style.width) to include a
branch like syncStatus === 'starting' ? 18 (or another appropriate initial
value) so that when sync.progress is not a number and status is 'starting' the
bar shows the intended initial width; keep the existing bounds/clamping
(Math.max/Math.min) for the numeric progress branch and ensure the order of
checks preserves numeric progress > 'starting' handling.
Comment on lines
+509
to
512
| } finally { | ||
| if (logPoll) clearInterval(logPoll); | ||
| await refreshLaunchLogs(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider wrapping final log refresh in try-catch.
If refreshLaunchLogs() throws in the finally block, the exception propagates and could prevent onComplete() from being called. Since the overlay is removed and app proceeds regardless, the refresh failure should be non-fatal.
✏️ Proposed fix
} finally {
if (logPoll) clearInterval(logPoll);
- await refreshLaunchLogs();
+ try {
+ await refreshLaunchLogs();
+ } catch {
+ // Non-fatal: overlay is about to be removed anyway
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/app.js` around lines 509 - 512, The final refresh call can throw and
block completion; wrap the await refreshLaunchLogs() in its own try-catch so any
error is caught and logged but not rethrown. Specifically, inside the finally
block that references logPoll and calls refreshLaunchLogs(), change it to
clearInterval(logPoll) as before and then try { await refreshLaunchLogs(); }
catch (err) { /* log error via console.error or existing logger */ } so failures
in refreshLaunchLogs() are non-fatal and do not prevent onComplete() or
subsequent cleanup from running.
Comment on lines
+1267
to
+1269
| .tracking-\[0\.2em\] { | ||
| --tw-tracking: 0.2em; | ||
| letter-spacing: 0.2em; |
There was a problem hiding this comment.
Regenerate or reformat this compiled CSS so stylelint passes.
Stylelint is already flagging Line 1269 and Line 1411 (declaration-empty-line-before). If this asset stays committed, the PR will keep failing lint until the generated output is regenerated from the source stylesheet/config or these declarations are reformatted.
Also applies to: 1409-1411
🧰 Tools
🪛 Stylelint (17.5.0)
[error] 1269-1269: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/output.css` around lines 1267 - 1269, The generated CSS contains
formatting that fails stylelint's declaration-empty-line-before rule around the
.tracking-\[0\.2em\] rule (and the nearby block around the same tracking
utility), so regenerate or reformat the compiled output: either re-run the
source stylesheet build (Tailwind/PostCSS) to produce a properly formatted
output or run stylelint --fix / apply the project's CSS formatter to remove or
add the required empty line before declarations to satisfy
declaration-empty-line-before for the .tracking-\[0\.2em\] rule and the
corresponding block referenced in the diff.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
src/components/swap/Coinswap.js (1)
941-952:⚠️ Potential issue | 🟠 MajorNormalize
swapData.makersbefore doing SVG layout math.
swapData.makersis still treated as trusted here. If it comes back as'3',totalNodesbecomes'31'; if it is0or non-integer, the later radius and arrow calculations produce degenerate geometry andNaNcoordinates.Suggested fix
- const actualMakers = swapData.makers; // Number of actual makers (e.g., 2) - const totalNodes = actualMakers + 1; + const normalizedMakerCount = Number(swapData.makers); + if (!Number.isInteger(normalizedMakerCount) || normalizedMakerCount < 1) { + return ` + <div class="p-4 text-center text-sm text-gray-500"> + Waiting for maker routing data... + </div> + `; + } + + const actualMakers = normalizedMakerCount; + const totalNodes = actualMakers + 1;As per coding guidelines, "Validate all user inputs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/Coinswap.js` around lines 941 - 952, Normalize and validate swapData.makers before any layout math: replace direct uses of swapData.makers with a sanitized value (e.g., const actualMakers = (() => { const n = Number(swapData.makers); return Number.isFinite(n) && n > 0 ? Math.floor(n) : 0; })()); then compute totalNodes, youHalf, makerHalf, youFont, makerFont, youRx, makerRx, maxNodeHalf, labelPad from that sanitized actualMakers and clamp it to a reasonable max if needed; ensure any downstream geometry uses this validated integer to avoid string concatenation, NaN, or negative values (refer to actualMakers, totalNodes, youHalf, makerHalf, youFont, makerFont, youRx, makerRx, maxNodeHalf, labelPad).src/components/swap/SwapReport.js (1)
568-576:⚠️ Potential issue | 🟠 MajorHandle the zero-maker case before building the layout.
This normalization still allows
actualMakers === 0. That makestotalNodes === 1, so the first circle branch blows up aroundMath.sin(Math.PI / totalNodes)and the arrow pass renders self-links withNaNendpoints. Return an empty state, or clamp to a minimum valid count, before continuing.Suggested fix
- const makersCount = Number(report.makersCount); - const actualMakers = Math.max( - 0, - Number.isFinite(makersCount) && makersCount > 0 - ? makersCount - : report.makerAddresses.length - ); + const makersCount = Number(report.makersCount); + const actualMakers = + Number.isInteger(makersCount) && makersCount > 0 + ? makersCount + : Array.isArray(report.makerAddresses) + ? report.makerAddresses.length + : 0; + + if (actualMakers < 1) { + return ` + <div class="py-8 text-center text-sm text-gray-500"> + No maker routing data available + </div> + `; + }As per coding guidelines, "Validate all user inputs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/SwapReport.js` around lines 568 - 576, The code in SwapReport normalizes makers but can still produce actualMakers === 0 which yields totalNodes === 1 and causes NaN in the circle/math layout (e.g., Math.sin(Math.PI / totalNodes)) and self-link arrows; fix by validating/clamping before layout: in the SwapReport component after computing actualMakers (from report.makersCount and report.makerAddresses) either return an empty/placeholder state when actualMakers === 0 or clamp totalNodes to a minimum valid value (e.g., ensure totalNodes = Math.max(2, actualMakers + 1)) so downstream math (circle branch, arrow path generation) never divides by zero or computes invalid trig values.src/components/swap/SwapHistory.js (1)
157-255:⚠️ Potential issue | 🟠 MajorThe new coercion helpers still aren’t on the live render path.
SwapHistoryComponent()keeps using its localbuildSwapHistoryList()and raw stats block, so these helpers never run. That leaves the oldswap.feePercentage?.toFixed(2)path at Line 390 in place, which still throws on string percentages from persisted reports. Either wire the component throughsummarizeSwapHistory()/buildSwapHistoryMarkup(), or normalizefeePercentageinnormalizeSwapReport().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/SwapHistory.js` around lines 157 - 255, SwapHistoryComponent is still using its local buildSwapHistoryList and raw stats, so summarizeSwapHistory/buildSwapHistoryMarkup never run and swap.feePercentage?.toFixed(2) still throws when feePercentage is a string; fix by normalizing feePercentage in normalizeSwapReport(): parseNumber or Number(swap.feePercentage) and store as a numeric value (fallback 0) and ensure normalizeSwapReport is called for reports used by SwapHistoryComponent; alternatively, wire SwapHistoryComponent to use summarizeSwapHistory() and buildSwapHistoryMarkup() instead of buildSwapHistoryList() so the coercion helpers run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api1.js`:
- Around line 117-136: getAllSwapReportPaths/getHistoricalSwapOutputMap
currently synchronously scans and parses all swap_reports/**/*.json on every
balance request, blocking the Electron main thread; change this to cache the
parsed historical swap index per wallet (keyed by wallet ID or active wallet
path) and only rebuild the cache when saveSwapReport writes a new report (or
when wallet switches). Concretely: scope the scan in getAllSwapReportPaths to
the active wallet directory instead of the global swap_reports root, add an
in-memory cache map (e.g., historicalSwapCache[walletId]) and a last-modified
marker, update getHistoricalSwapOutputMap to return cached data when valid, and
have saveSwapReport invalidate or update that wallet's cache after writing a
report so UI balance requests no longer trigger synchronous full-disk rescans.
- Around line 638-640: The three console.log calls printing balance, normalized,
and debug currently expose sensitive wallet metadata (notably
debug.historicalSwapMatches); replace these with logs that redact or omit
per-wallet details—e.g., log only aggregate counts/totals or a sanitized
summary—and if you need full details keep them behind a dedicated diagnostic
flag (e.g., only emit debug when DIAGNOSTICS_ENABLED) or produce a redacted copy
of debug where fields like historicalSwapMatches are replaced with counts or
masked addresses; update the console.log statements that reference balance,
normalized, and debug to use the sanitized/aggregated output instead.
- Around line 141-156: The current parser in the getAllSwapReportPaths loop
stores swap outputs in swapOutputs keyed only by address, causing later
address-only matches to misclassify non-swap UTXOs; change the key to include
amount (and prefer txid+vout when present) when populating outputSwapUtxos in
the loop that iterates outputSwapUtxos (use the same composite key logic used
later when checking historicalSwapOutputs.has), e.g., build a composite key from
address and toNumber(amount) or from report-provided txid and vout if available;
update any lookup sites that call historicalSwapOutputs.has(…) to compute the
same composite key so matching is performed on (address,amount) or (txid,vout)
instead of address-only.
- Around line 635-645: taker:getBalance now returns normalized numbers but
taker:checkSwapLiquidity still computes spendable/regular/swap from the raw
native payload; update taker:checkSwapLiquidity to call
normalizeBalancePayload(balance, rawUtxos) (same as used in the taker:getBalance
path) and use the returned normalized values (e.g., normalized.spendable,
normalized.regular, normalized.swap) instead of deriving from the raw payload so
both IPC routes report the same figures.
In `@src/components/swap/Coinswap.js`:
- Around line 1257-1262: The template conditionally omits the <div
id="transaction-list"> when isV2 is true, but updateTxList() still expects that
element for V2 updates; restore visibility by always rendering the container
(move or remove the isV2 conditional around the transaction-list block) so
`#transaction-list` exists for both V1 and V2, or alternatively update
updateTxList() to not early-return for the V2 branch; reference updateTxList,
the isV2 flag, and the element id "transaction-list" when making the change.
In `@src/components/swap/SwapHistory.js`:
- Around line 145-154: SwapHistoryComponent() currently calls loadSwapHistory()
directly and will crash because loadSwapHistory() now rethrows on refresh
failures; wrap the call to loadSwapHistory() in a try/catch inside
SwapHistoryComponent(), handle the error by setting swap history to an empty
array and toggling/loading an error/fallback UI state (e.g., setSwapHistory([]);
setIsLoading(false); setHasError(true)), log the error with console.error, and
avoid rethrowing so the component can render its fallback UI instead of
rejecting during navigation.
In `@src/components/swap/SwapReport.js`:
- Around line 144-151: feePercentage can be a string and the current || chain
treats 0 as missing; coerce to a number and use nullish-coalescing so 0 is
preserved: compute feePercentage by first attempting to parseNumber from
swapReport.feePercentage, swapReport.fee_percentage, nestedReport.feePercentage,
nestedReport.fee_percentage (e.g. Number(...) or parseFloat(...)), then if all
are null/undefined use the computed expression ((rawTotalFee /
normalizedTargetAmount) * 100) when normalizedTargetAmount > 0, and finally
ensure the value is a safe number (fallback to 0 if NaN); update the assignment
used to build report.feePercentage accordingly so downstream
report.feePercentage.toFixed(2) always works.
In `@src/js/app.js`:
- Around line 495-499: The catch block that currently calls console.error('Sync
polling error:', err) should follow project logging convention by prefixing the
message with the warning emoji and context; update the error log in the catch
handling for the sync polling code to use console.error('⚠️ Sync polling
error:', err) (leave clearInterval(poll) and resolve() as-is).
---
Duplicate comments:
In `@src/components/swap/Coinswap.js`:
- Around line 941-952: Normalize and validate swapData.makers before any layout
math: replace direct uses of swapData.makers with a sanitized value (e.g., const
actualMakers = (() => { const n = Number(swapData.makers); return
Number.isFinite(n) && n > 0 ? Math.floor(n) : 0; })()); then compute totalNodes,
youHalf, makerHalf, youFont, makerFont, youRx, makerRx, maxNodeHalf, labelPad
from that sanitized actualMakers and clamp it to a reasonable max if needed;
ensure any downstream geometry uses this validated integer to avoid string
concatenation, NaN, or negative values (refer to actualMakers, totalNodes,
youHalf, makerHalf, youFont, makerFont, youRx, makerRx, maxNodeHalf, labelPad).
In `@src/components/swap/SwapHistory.js`:
- Around line 157-255: SwapHistoryComponent is still using its local
buildSwapHistoryList and raw stats, so
summarizeSwapHistory/buildSwapHistoryMarkup never run and
swap.feePercentage?.toFixed(2) still throws when feePercentage is a string; fix
by normalizing feePercentage in normalizeSwapReport(): parseNumber or
Number(swap.feePercentage) and store as a numeric value (fallback 0) and ensure
normalizeSwapReport is called for reports used by SwapHistoryComponent;
alternatively, wire SwapHistoryComponent to use summarizeSwapHistory() and
buildSwapHistoryMarkup() instead of buildSwapHistoryList() so the coercion
helpers run.
In `@src/components/swap/SwapReport.js`:
- Around line 568-576: The code in SwapReport normalizes makers but can still
produce actualMakers === 0 which yields totalNodes === 1 and causes NaN in the
circle/math layout (e.g., Math.sin(Math.PI / totalNodes)) and self-link arrows;
fix by validating/clamping before layout: in the SwapReport component after
computing actualMakers (from report.makersCount and report.makerAddresses)
either return an empty/placeholder state when actualMakers === 0 or clamp
totalNodes to a minimum valid value (e.g., ensure totalNodes = Math.max(2,
actualMakers + 1)) so downstream math (circle branch, arrow path generation)
never divides by zero or computes invalid trig values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4eacc844-f2d0-4d56-98ec-74f502d31b6d
📒 Files selected for processing (5)
api1.jssrc/components/swap/Coinswap.jssrc/components/swap/SwapHistory.jssrc/components/swap/SwapReport.jssrc/js/app.js
Comment on lines
+117
to
+136
| function getAllSwapReportPaths() { | ||
| const reportsRoot = path.join(api1State.DATA_DIR, 'swap_reports'); | ||
| const reportPaths = []; | ||
|
|
||
| function walk(dirPath) { | ||
| if (!fs.existsSync(dirPath)) return; | ||
|
|
||
| for (const entry of fs.readdirSync(dirPath, { withFileTypes: true })) { | ||
| const fullPath = path.join(dirPath, entry.name); | ||
| if (entry.isDirectory()) { | ||
| walk(fullPath); | ||
| } else if (entry.isFile() && entry.name.endsWith('.json')) { | ||
| reportPaths.push(fullPath); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| walk(reportsRoot); | ||
| return reportPaths; | ||
| } |
There was a problem hiding this comment.
Don't rebuild the historical swap index inside the balance hot path.
getHistoricalSwapOutputMap() synchronously walks and parses every swap_reports/**/*.json file each time balance is requested. Because taker:getBalance runs on Electron's main process and is hit on initial load, wallets with many reports will block the UI here. Cache this per wallet and invalidate it when saveSwapReport() writes a new report, or at minimum scope the scan to the active wallet directory.
Also applies to: 138-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api1.js` around lines 117 - 136,
getAllSwapReportPaths/getHistoricalSwapOutputMap currently synchronously scans
and parses all swap_reports/**/*.json on every balance request, blocking the
Electron main thread; change this to cache the parsed historical swap index per
wallet (keyed by wallet ID or active wallet path) and only rebuild the cache
when saveSwapReport writes a new report (or when wallet switches). Concretely:
scope the scan in getAllSwapReportPaths to the active wallet directory instead
of the global swap_reports root, add an in-memory cache map (e.g.,
historicalSwapCache[walletId]) and a last-modified marker, update
getHistoricalSwapOutputMap to return cached data when valid, and have
saveSwapReport invalidate or update that wallet's cache after writing a report
so UI balance requests no longer trigger synchronous full-disk rescans.
Comment on lines
+141
to
+156
| for (const reportPath of getAllSwapReportPaths()) { | ||
| try { | ||
| const report = JSON.parse(fs.readFileSync(reportPath, 'utf8')); | ||
| const outputSwapUtxos = | ||
| report.output_swap_utxos || | ||
| report.outputSwapUtxos || | ||
| report.report?.output_swap_utxos || | ||
| report.report?.outputSwapUtxos || | ||
| []; | ||
|
|
||
| outputSwapUtxos.forEach((entry) => { | ||
| if (!Array.isArray(entry) || entry.length < 2) return; | ||
| const [amount, address] = entry; | ||
| if (!address) return; | ||
| swapOutputs.set(address, toNumber(amount, 0)); | ||
| }); |
There was a problem hiding this comment.
Match historical swap outputs on more than the address.
The parser records amount, but the matcher only checks historicalSwapOutputs.has(utxoEntry?.address). Any later non-swap UTXO paid to the same address will be reclassified as swap, inflating normalized.swap and shrinking regular. Match on (address, amount) at minimum, or (txid, vout) if the report format supports it.
🛠️ Suggested fix
function getHistoricalSwapOutputMap() {
const swapOutputs = new Map();
@@
outputSwapUtxos.forEach((entry) => {
if (!Array.isArray(entry) || entry.length < 2) return;
const [amount, address] = entry;
if (!address) return;
- swapOutputs.set(address, toNumber(amount, 0));
+ const normalizedAmount = toNumber(amount, 0);
+ const amounts = swapOutputs.get(address) || new Set();
+ amounts.add(normalizedAmount);
+ swapOutputs.set(address, amounts);
});
@@
const matchedHistoricalSwapUtxos = rawUtxos.filter(([utxoEntry]) =>
- historicalSwapOutputs.has(utxoEntry?.address)
+ historicalSwapOutputs
+ .get(utxoEntry?.address)
+ ?.has(toNumber(utxoEntry?.amount?.sats, 0))
);Also applies to: 213-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api1.js` around lines 141 - 156, The current parser in the
getAllSwapReportPaths loop stores swap outputs in swapOutputs keyed only by
address, causing later address-only matches to misclassify non-swap UTXOs;
change the key to include amount (and prefer txid+vout when present) when
populating outputSwapUtxos in the loop that iterates outputSwapUtxos (use the
same composite key logic used later when checking historicalSwapOutputs.has),
e.g., build a composite key from address and toNumber(amount) or from
report-provided txid and vout if available; update any lookup sites that call
historicalSwapOutputs.has(…) to compute the same composite key so matching is
performed on (address,amount) or (txid,vout) instead of address-only.
Comment on lines
+635
to
645
| const rawUtxos = api1State.takerInstance.listAllUtxoSpendInfo(); | ||
| const { normalized, debug } = normalizeBalancePayload(balance, rawUtxos); | ||
|
|
||
| console.log('💰 Raw taker balance payload:', balance); | ||
| console.log('💰 Normalized taker balance payload:', normalized); | ||
| console.log('💰 Balance derivation details:', debug); | ||
|
|
||
| return { | ||
| success: true, | ||
| balance: { | ||
| spendable: balance.spendable, | ||
| regular: balance.regular, | ||
| swap: balance.swap, | ||
| contract: balance.contract, | ||
| fidelity: balance.fidelity, | ||
| }, | ||
| balance: normalized, | ||
| }; |
There was a problem hiding this comment.
Keep both balance IPCs on the same normalization path.
After this change taker:getBalance returns normalized numbers, but taker:checkSwapLiquidity later in this file still derives spendable, regular, and swap from the raw native payload. The same wallet can now report different balance figures depending on which IPC route the renderer calls. Reuse normalizeBalancePayload() there as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api1.js` around lines 635 - 645, taker:getBalance now returns normalized
numbers but taker:checkSwapLiquidity still computes spendable/regular/swap from
the raw native payload; update taker:checkSwapLiquidity to call
normalizeBalancePayload(balance, rawUtxos) (same as used in the taker:getBalance
path) and use the returned normalized values (e.g., normalized.spendable,
normalized.regular, normalized.swap) instead of deriving from the raw payload so
both IPC routes report the same figures.
Comment on lines
+638
to
+640
| console.log('💰 Raw taker balance payload:', balance); | ||
| console.log('💰 Normalized taker balance payload:', normalized); | ||
| console.log('💰 Balance derivation details:', debug); |
There was a problem hiding this comment.
Redact wallet metadata from these logs.
debug.historicalSwapMatches contains wallet addresses, amounts, and spend types, and these logs are emitted on every balance fetch. That exposes wallet history in main-process logs and any collected diagnostics. Keep only aggregate counts here or gate detailed output behind a dedicated diagnostic switch.
🛡️ Suggested fix
- console.log('💰 Raw taker balance payload:', balance);
- console.log('💰 Normalized taker balance payload:', normalized);
- console.log('💰 Balance derivation details:', debug);
+ console.log('💰 Normalized taker balance payload:', normalized);
+ console.log('💰 Balance derivation summary:', {
+ historicalSwapBalance: debug.historicalSwapBalance,
+ historicalSwapMatchCount: debug.historicalSwapMatches.length,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log('💰 Raw taker balance payload:', balance); | |
| console.log('💰 Normalized taker balance payload:', normalized); | |
| console.log('💰 Balance derivation details:', debug); | |
| console.log('💰 Normalized taker balance payload:', normalized); | |
| console.log('💰 Balance derivation summary:', { | |
| historicalSwapBalance: debug.historicalSwapBalance, | |
| historicalSwapMatchCount: debug.historicalSwapMatches.length, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api1.js` around lines 638 - 640, The three console.log calls printing
balance, normalized, and debug currently expose sensitive wallet metadata
(notably debug.historicalSwapMatches); replace these with logs that redact or
omit per-wallet details—e.g., log only aggregate counts/totals or a sanitized
summary—and if you need full details keep them behind a dedicated diagnostic
flag (e.g., only emit debug when DIAGNOSTICS_ENABLED) or produce a redacted copy
of debug where fields like historicalSwapMatches are replaced with counts or
masked addresses; update the console.log statements that reference balance,
normalized, and debug to use the sanitized/aggregated output instead.
Comment on lines
+1257
to
+1262
| ${!isV2 ? ` | ||
| <div class="bg-[#1a2332] rounded-xl p-4"> | ||
| <p class="text-xs text-gray-500 uppercase tracking-wide mb-2">Hop Transactions</p> | ||
| <div id="transaction-list" class="space-y-1.5 max-h-28 overflow-y-auto"></div> | ||
| </div> | ||
| ` | ||
| : '' | ||
| } | ||
| ` : ''} |
There was a problem hiding this comment.
Don’t remove #transaction-list for V2 swaps.
updateTxList() still has a V2 branch, but this template never renders the container when isV2 is true. Every V2 tx update then returns early at Line 875, so outgoing/incoming txids and statuses are never shown.
Suggested fix
- ${!isV2 ? `
- <div class="bg-[`#1a2332`] rounded-xl p-4">
- <p class="text-xs text-gray-500 uppercase tracking-wide mb-2">Hop Transactions</p>
- <div id="transaction-list" class="space-y-1.5 max-h-28 overflow-y-auto"></div>
- </div>
- ` : ''}
+ <div class="bg-[`#1a2332`] rounded-xl p-4">
+ <p class="text-xs text-gray-500 uppercase tracking-wide mb-2">
+ ${isV2 ? 'Contract Transactions' : 'Hop Transactions'}
+ </p>
+ <div id="transaction-list" class="space-y-1.5 max-h-28 overflow-y-auto"></div>
+ </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/swap/Coinswap.js` around lines 1257 - 1262, The template
conditionally omits the <div id="transaction-list"> when isV2 is true, but
updateTxList() still expects that element for V2 updates; restore visibility by
always rendering the container (move or remove the isV2 conditional around the
transaction-list block) so `#transaction-list` exists for both V1 and V2, or
alternatively update updateTxList() to not early-return for the V2 branch;
reference updateTxList, the isV2 flag, and the element id "transaction-list"
when making the change.
Comment on lines
+145
to
+154
| } else { | ||
| swapHistory = []; | ||
| throw new Error(result.error || 'Failed to load swap history'); | ||
| } | ||
| } catch (error) { | ||
| swapHistory = []; | ||
| console.error('Failed to load swap history:', error); | ||
| throw error; | ||
| } | ||
| return swapHistory; |
There was a problem hiding this comment.
SwapHistoryComponent() needs to handle this new throw path.
loadSwapHistory() now rethrows on refresh failures, but the same file still does a bare await loadSwapHistory() at Line 265. Direct navigation into the history screen will reject before any fallback UI is rendered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/swap/SwapHistory.js` around lines 145 - 154,
SwapHistoryComponent() currently calls loadSwapHistory() directly and will crash
because loadSwapHistory() now rethrows on refresh failures; wrap the call to
loadSwapHistory() in a try/catch inside SwapHistoryComponent(), handle the error
by setting swap history to an empty array and toggling/loading an error/fallback
UI state (e.g., setSwapHistory([]); setIsLoading(false); setHasError(true)), log
the error with console.error, and avoid rethrowing so the component can render
its fallback UI instead of rejecting during navigation.
Comment on lines
+495
to
+499
| } catch (err) { | ||
| console.error('Sync polling error:', err); | ||
| clearInterval(poll); | ||
| resolve(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Error logging should follow the project convention.
The error is now properly logged (good fix!), but the format should include the emoji prefix per coding guidelines.
✏️ Suggested change
} catch (err) {
- console.error('Sync polling error:', err);
+ console.error('⚠️ Sync polling error:', err);
clearInterval(poll);
resolve();
}As per coding guidelines: "Log errors with context using console.error('
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| console.error('Sync polling error:', err); | |
| clearInterval(poll); | |
| resolve(); | |
| } | |
| } catch (err) { | |
| console.error('⚠️ Sync polling error:', err); | |
| clearInterval(poll); | |
| resolve(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/app.js` around lines 495 - 499, The catch block that currently calls
console.error('Sync polling error:', err) should follow project logging
convention by prefixing the message with the warning emoji and context; update
the error log in the catch handling for the sync polling code to use
console.error('⚠️ Sync polling error:', err) (leave clearInterval(poll) and
resolve() as-is).
keraliss
force-pushed
the
swap_layout_fix
branch
from
250fdd0 to
cea2ec1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
api1.js (4)
638-640:⚠️ Potential issue | 🟠 MajorRedact the per-UTXO balance debug logs.
debug.historicalSwapMatchesincludes addresses, amounts, and spend types, and this is emitted on every balance fetch. Keep only aggregate counts/totals here or gate the detailed object behind an explicit diagnostic flag.As per coding guidelines "Never log sensitive data (passwords, private keys)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api1.js` around lines 638 - 640, The current console.log calls (printing balance, normalized and debug, including debug.historicalSwapMatches) emit per-UTXO sensitive data; remove or redact those detailed logs and instead log only aggregate counts/totals (e.g., total UTXOs, total amount) or gate the full detailed object behind an explicit diagnostic flag (e.g., ENABLE_DETAILED_BALANCE_LOGS) so detailed addresses/amounts are never logged in normal operation; update the logging in the block that prints '💰 Raw taker balance payload:', '💰 Normalized taker balance payload:', and '💰 Balance derivation details:' accordingly.
117-166:⚠️ Potential issue | 🟠 MajorCache and scope historical swap report indexing off the balance hot path.
normalizeBalancePayload()now reaches this code during everytaker:getBalance, so the main process synchronously walks and parses everyswap_reports/**/*.jsonfile on initial load. Wallets with real history will stall the UI here. Cache the parsed index per wallet and invalidate it fromsaveSwapReport(), or at minimum restrict the scan toswap_reports/<currentWalletName>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api1.js` around lines 117 - 166, getAllSwapReportPaths/getHistoricalSwapOutputMap currently synchronously scans and parses all swap_reports/**/*.json on every balance hot-path call (via normalizeBalancePayload), causing UI stalls; restrict the scan to the current wallet folder (e.g. swap_reports/<currentWalletName>) or implement a per-wallet cached Map of parsed outputs keyed by wallet name and expose an invalidate function that saveSwapReport() calls after writing. Specifically: modify getAllSwapReportPaths/getHistoricalSwapOutputMap to accept a walletName parameter and only walk that subdirectory (or return cached data if present), add a simple in-memory cache keyed by walletName, and update saveSwapReport() to clear/invalidate that wallet's cache when a report is saved so the cache stays fresh.
151-156:⚠️ Potential issue | 🟠 MajorMatch historical swap outputs on a coin identifier, not just the address.
swapOutputs.set(address, ...)plushistoricalSwapOutputs.has(utxoEntry?.address)will reclassify any later non-swap UTXO paid to the same address asswap. Usetxid:voutwhen the report contains it, otherwise at least(address, amount)with multiplicity tracking, before shifting value out ofregular.Based on learnings "UTXO selection logic correctness".
Also applies to: 213-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api1.js` around lines 151 - 156, The code currently keys swapOutputs by address (outputSwapUtxos -> swapOutputs.set(address, ...)) which causes later non-swap UTXOs to be misclassified when historicalSwapOutputs.has(utxoEntry?.address) is used; change the keying to use a coin identifier: prefer "txid:vout" when the report provides txid and vout, otherwise use a composite key of "address:amount" and track multiplicity (count duplicates) so lookups and removals use the same identifier; update both the population sites (outputSwapUtxos handling and the similar block at lines ~213-215) to set and check historicalSwapOutputs using these coin keys and decrement multiplicity when consuming a matched swap so values aren’t shifted out of `regular`.
635-645:⚠️ Potential issue | 🟠 MajorKeep
checkSwapLiquidityon the same normalization path.After this change
taker:getBalancereturnsnormalized, buttaker:checkSwapLiquiditylater in this file still derivesspendable,regular, andswapfrom rawgetBalances(). The same wallet can now surface different balance figures depending on which IPC the renderer calls. ReusenormalizeBalancePayload(balance, listAllUtxoSpendInfo())there too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api1.js` around lines 635 - 645, taker:getBalance now returns the normalized payload but taker:checkSwapLiquidity still computes spendable/regular/swap from raw getBalances(), causing inconsistent figures; update taker:checkSwapLiquidity to call normalizeBalancePayload(getBalances(), listAllUtxoSpendInfo()) (or the same balance + rawUtxos pair used in the getBalance path) and then derive spendable/regular/swap from the returned normalized object (e.g., normalized.spendable, normalized.regular, normalized.swap) instead of computing them from the raw getBalances() output, ensuring both endpoints use normalizeBalancePayload consistently.src/components/swap/Coinswap.js (2)
1257-1262:⚠️ Potential issue | 🟠 MajorDon't remove
#transaction-listfor Taproot swaps.
updateTxList()still has a V2 branch and is called during initialization, but this template omits the container whenisV2is true. Outgoing/incoming txids and statuses never render for V2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/Coinswap.js` around lines 1257 - 1262, The transaction list container (`#transaction-list`) is currently omitted when isV2 is true which prevents updateTxList() from rendering V2 (Taproot) txids and statuses; update the template in Coinswap.js to always render the <div id="transaction-list"> container (remove it from the isV2 conditional or duplicate a minimal container for V2) so updateTxList() can find and populate it for both legacy and V2 flows, ensuring any styling differences for V2 are applied via classes or conditional wrappers rather than removing the element entirely.
941-953:⚠️ Potential issue | 🟠 MajorCoerce and validate
swapData.makersbefore building the SVG.
const totalNodes = actualMakers + 1will concatenate whenswapData.makersis a persisted string, and0/NaNlater drives the adaptive layout into Infinity/NaN coordinates. Parse it once, clamp to a minimum of 1, and show a fallback state when routing data is missing.As per coding guidelines "Validate all user inputs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/Coinswap.js` around lines 941 - 953, The code uses swapData.makers directly which can be a string and cause concatenation/NaN issues; in the Coinswap component coerce and validate it once (e.g., parseInt/Number on swapData.makers), clamp to a minimum of 1 (so actualMakers = Math.max(1, parsed)), and then compute totalNodes = actualMakers + 1; additionally detect missing/invalid routing data and render a safe fallback state/UI instead of building the SVG when parsed value is NaN or routing info absent. Update the places referencing actualMakers, totalNodes, youHalf, makerHalf, etc., to use the validated actualMakers value.src/components/swap/SwapHistory.js (1)
145-154:⚠️ Potential issue | 🟠 MajorThis new throw path still crashes direct history navigation.
loadSwapHistory()now rethrows on refresh failures, butSwapHistoryComponent()still does a bareawait loadSwapHistory()at Line 265. On any backend error the component rejects before it can render fallback UI. Either keep this helper returning[]or catch the error in the component in the same change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/SwapHistory.js` around lines 145 - 154, The new rethrow in loadSwapHistory causes SwapHistoryComponent's bare await to reject and crash navigation; wrap the call to loadSwapHistory() inside a try/catch in SwapHistoryComponent (where it currently does `await loadSwapHistory()`), on error set the local swapHistory to [] (or a safe fallback) and avoid rethrowing so the component can render its fallback/loading UI; alternatively remove the `throw` from loadSwapHistory and return [] on failure—prefer catching in SwapHistoryComponent around the call to loadSwapHistory() to preserve the helper's error semantics.src/components/swap/SwapReport.js (1)
568-576:⚠️ Potential issue | 🟠 MajorGuard the no-maker case before entering the adaptive layout.
The numeric coercion helps, but
actualMakerscan still be0for older or incomplete reports. That makestotalNodes === 1, and the circle branch later divides byMath.sin(Math.PI / totalNodes) === 0, producing Infinity/NaN SVG sizes instead of a fallback report view.As per coding guidelines "Validate all user inputs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/SwapReport.js` around lines 568 - 576, The code computes makersCount/actualMakers and then totalNodes which can be 1, causing a divide-by-zero in the circle layout; add a guard right after computing actualMakers (or before using totalNodes) to handle the no-maker case: if actualMakers === 0 (or totalNodes < 2) either return/render the existing fallback report view instead of entering the adaptive/circle layout or coerce totalNodes to a safe minimum (e.g., set totalNodes = Math.max(2, actualMakers + 1)); update references around makersCount, actualMakers and totalNodes in SwapReport.js so the circle math never runs when totalNodes < 2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/app.js`:
- Around line 436-437: The current early return after calling
window.api.logs.get(120) silently ignores failures; change the conditional
handling around the result of window.api.logs.get so that when data.success is
false you log the error with context (e.g., console.error('⚠️ logs:get failed:',
data.error)) and when data.logs is not an array you also log a clear diagnostic
(e.g., console.error('⚠️ Unexpected logs payload:', data)); keep the existing
return behavior only after logging so the panel no longer silently stays on
"Waiting for startup logs...".
- Around line 463-490: The code is using sync.message but the IPC returns errors
on status.error (lookup failure) and sync.error (sync failure); update the
syncMessage computation in the getSyncStatus handling to prefer status.error
when present, then sync.error, then sync.message, falling back to the existing
status-based strings; likewise ensure the done condition treats status.success
=== false (status.error present) as finished and uses the appropriate error
message from status.error instead of the generic 'Syncing offerbook...' by
referencing status.error and sync.error alongside sync.message in the logic
around status, sync, syncMessage and done.
---
Duplicate comments:
In `@api1.js`:
- Around line 638-640: The current console.log calls (printing balance,
normalized and debug, including debug.historicalSwapMatches) emit per-UTXO
sensitive data; remove or redact those detailed logs and instead log only
aggregate counts/totals (e.g., total UTXOs, total amount) or gate the full
detailed object behind an explicit diagnostic flag (e.g.,
ENABLE_DETAILED_BALANCE_LOGS) so detailed addresses/amounts are never logged in
normal operation; update the logging in the block that prints '💰 Raw taker
balance payload:', '💰 Normalized taker balance payload:', and '💰 Balance
derivation details:' accordingly.
- Around line 117-166: getAllSwapReportPaths/getHistoricalSwapOutputMap
currently synchronously scans and parses all swap_reports/**/*.json on every
balance hot-path call (via normalizeBalancePayload), causing UI stalls; restrict
the scan to the current wallet folder (e.g. swap_reports/<currentWalletName>) or
implement a per-wallet cached Map of parsed outputs keyed by wallet name and
expose an invalidate function that saveSwapReport() calls after writing.
Specifically: modify getAllSwapReportPaths/getHistoricalSwapOutputMap to accept
a walletName parameter and only walk that subdirectory (or return cached data if
present), add a simple in-memory cache keyed by walletName, and update
saveSwapReport() to clear/invalidate that wallet's cache when a report is saved
so the cache stays fresh.
- Around line 151-156: The code currently keys swapOutputs by address
(outputSwapUtxos -> swapOutputs.set(address, ...)) which causes later non-swap
UTXOs to be misclassified when historicalSwapOutputs.has(utxoEntry?.address) is
used; change the keying to use a coin identifier: prefer "txid:vout" when the
report provides txid and vout, otherwise use a composite key of "address:amount"
and track multiplicity (count duplicates) so lookups and removals use the same
identifier; update both the population sites (outputSwapUtxos handling and the
similar block at lines ~213-215) to set and check historicalSwapOutputs using
these coin keys and decrement multiplicity when consuming a matched swap so
values aren’t shifted out of `regular`.
- Around line 635-645: taker:getBalance now returns the normalized payload but
taker:checkSwapLiquidity still computes spendable/regular/swap from raw
getBalances(), causing inconsistent figures; update taker:checkSwapLiquidity to
call normalizeBalancePayload(getBalances(), listAllUtxoSpendInfo()) (or the same
balance + rawUtxos pair used in the getBalance path) and then derive
spendable/regular/swap from the returned normalized object (e.g.,
normalized.spendable, normalized.regular, normalized.swap) instead of computing
them from the raw getBalances() output, ensuring both endpoints use
normalizeBalancePayload consistently.
In `@src/components/swap/Coinswap.js`:
- Around line 1257-1262: The transaction list container (`#transaction-list`) is
currently omitted when isV2 is true which prevents updateTxList() from rendering
V2 (Taproot) txids and statuses; update the template in Coinswap.js to always
render the <div id="transaction-list"> container (remove it from the isV2
conditional or duplicate a minimal container for V2) so updateTxList() can find
and populate it for both legacy and V2 flows, ensuring any styling differences
for V2 are applied via classes or conditional wrappers rather than removing the
element entirely.
- Around line 941-953: The code uses swapData.makers directly which can be a
string and cause concatenation/NaN issues; in the Coinswap component coerce and
validate it once (e.g., parseInt/Number on swapData.makers), clamp to a minimum
of 1 (so actualMakers = Math.max(1, parsed)), and then compute totalNodes =
actualMakers + 1; additionally detect missing/invalid routing data and render a
safe fallback state/UI instead of building the SVG when parsed value is NaN or
routing info absent. Update the places referencing actualMakers, totalNodes,
youHalf, makerHalf, etc., to use the validated actualMakers value.
In `@src/components/swap/SwapHistory.js`:
- Around line 145-154: The new rethrow in loadSwapHistory causes
SwapHistoryComponent's bare await to reject and crash navigation; wrap the call
to loadSwapHistory() inside a try/catch in SwapHistoryComponent (where it
currently does `await loadSwapHistory()`), on error set the local swapHistory to
[] (or a safe fallback) and avoid rethrowing so the component can render its
fallback/loading UI; alternatively remove the `throw` from loadSwapHistory and
return [] on failure—prefer catching in SwapHistoryComponent around the call to
loadSwapHistory() to preserve the helper's error semantics.
In `@src/components/swap/SwapReport.js`:
- Around line 568-576: The code computes makersCount/actualMakers and then
totalNodes which can be 1, causing a divide-by-zero in the circle layout; add a
guard right after computing actualMakers (or before using totalNodes) to handle
the no-maker case: if actualMakers === 0 (or totalNodes < 2) either
return/render the existing fallback report view instead of entering the
adaptive/circle layout or coerce totalNodes to a safe minimum (e.g., set
totalNodes = Math.max(2, actualMakers + 1)); update references around
makersCount, actualMakers and totalNodes in SwapReport.js so the circle math
never runs when totalNodes < 2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3cb87fbc-5940-44b9-b7bb-29aef41d01d5
📒 Files selected for processing (5)
api1.jssrc/components/swap/Coinswap.jssrc/components/swap/SwapHistory.jssrc/components/swap/SwapReport.jssrc/js/app.js
Comment on lines
+436
to
+437
| const data = await window.api.logs.get(120); | ||
| if (!data.success || !Array.isArray(data.logs)) return; |
There was a problem hiding this comment.
Don't silently ignore logs:get failures.
This IPC reports read problems via { success: false, error }. The current early return leaves the panel stuck on “Waiting for startup logs...” with no diagnostic when the backend cannot read debug.log.
✏️ Suggested change
const data = await window.api.logs.get(120);
- if (!data.success || !Array.isArray(data.logs)) return;
+ if (!data.success) {
+ console.error('⚠️ Failed to refresh launch sync logs:', data.error);
+ return;
+ }
+ if (!Array.isArray(data.logs)) return;As per coding guidelines: "Log errors with context using console.error('
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/app.js` around lines 436 - 437, The current early return after calling
window.api.logs.get(120) silently ignores failures; change the conditional
handling around the result of window.api.logs.get so that when data.success is
false you log the error with context (e.g., console.error('⚠️ logs:get failed:',
data.error)) and when data.logs is not an array you also log a clear diagnostic
(e.g., console.error('⚠️ Unexpected logs payload:', data)); keep the existing
return behavior only after logging so the panel no longer silently stays on
"Waiting for startup logs...".
Comment on lines
463
to
+490
| const status = await window.api.taker.getSyncStatus(syncId); | ||
| const done = !status.success || status.sync.status === 'completed' || status.sync.status === 'failed'; | ||
| if (done) { clearInterval(poll); resolve(); } | ||
| } catch { clearInterval(poll); resolve(); } | ||
| const sync = status.sync || {}; | ||
| const syncStatus = sync.status || 'syncing'; | ||
| const syncMessage = | ||
| sync.message || | ||
| (syncStatus === 'completed' | ||
| ? 'Offerbook ready' | ||
| : syncStatus === 'failed' | ||
| ? 'Sync failed' | ||
| : 'Syncing offerbook...'); | ||
|
|
||
| if (statusLabel) statusLabel.textContent = syncMessage; | ||
| if (progressBar) { | ||
| const nextWidth = | ||
| typeof sync.progress === 'number' | ||
| ? Math.max(18, Math.min(100, sync.progress)) | ||
| : syncStatus === 'completed' | ||
| ? 100 | ||
| : syncStatus === 'failed' | ||
| ? 100 | ||
| : 76; | ||
| progressBar.style.width = `${nextWidth}%`; | ||
| } | ||
|
|
||
| const done = | ||
| !status.success || | ||
| syncStatus === 'completed' || | ||
| syncStatus === 'failed'; |
There was a problem hiding this comment.
Use the error fields that getSyncStatus actually returns.
The current payload exposes status.error when the lookup fails and sync.error when the sync itself fails. Because this branch reads sync.message instead, startup failures collapse to generic text and can briefly render as “Syncing offerbook...” even after the IPC already returned an error.
✏️ Suggested change
const status = await window.api.taker.getSyncStatus(syncId);
+ if (!status.success) {
+ if (statusLabel) {
+ statusLabel.textContent =
+ status.error || 'Unable to read sync status';
+ }
+ if (progressBar) progressBar.style.width = '100%';
+ clearInterval(poll);
+ resolve();
+ return;
+ }
const sync = status.sync || {};
const syncStatus = sync.status || 'syncing';
const syncMessage =
- sync.message ||
- (syncStatus === 'completed'
- ? 'Offerbook ready'
- : syncStatus === 'failed'
- ? 'Sync failed'
- : 'Syncing offerbook...');
+ syncStatus === 'failed'
+ ? sync.error || 'Sync failed'
+ : syncStatus === 'completed'
+ ? 'Offerbook ready'
+ : 'Syncing offerbook...';
@@
- const done =
- !status.success ||
- syncStatus === 'completed' ||
- syncStatus === 'failed';
+ const done =
+ syncStatus === 'completed' ||
+ syncStatus === 'failed';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/js/app.js` around lines 463 - 490, The code is using sync.message but the
IPC returns errors on status.error (lookup failure) and sync.error (sync
failure); update the syncMessage computation in the getSyncStatus handling to
prefer status.error when present, then sync.error, then sync.message, falling
back to the existing status-based strings; likewise ensure the done condition
treats status.success === false (status.error present) as finished and uses the
appropriate error message from status.error instead of the generic 'Syncing
offerbook...' by referencing status.error and sync.error alongside sync.message
in the logic around status, sync, syncMessage and done.
Merged
This was referenced Apr 18, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Style