update readme and other nits#81
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR updates distribution artifact naming (TakerApp → CoinswapTaker), enhances timestamp normalization across swap-related components with new helper functions, improves Tor endpoint display formatting, adjusts initialization behavior in the receive component, refines documentation, and adds responsive CSS utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api1.js (1)
301-318:⚠️ Potential issue | 🟠 MajorRead
startTimestamp/endTimestampfrom flat reports too.
nestedReportis already normalized torawReport.report ?? rawReport, but this lookup still hardcodesrawReport.report?.startTimestamp/endTimestamp. Flat report files that only carry those aliases at the top level will fall back tomtimeMs/null, which then skews history ordering and duration inSwapHistory.js.🛠️ Suggested fix
const rawCompletedAt = - rawReport.completedAt || - rawReport.completed_at || - rawReport.report?.completedAt || - rawReport.report?.completed_at || - rawReport.report?.endTimestamp || - rawReport.report?.end_timestamp || + rawReport.completedAt ?? + rawReport.completed_at ?? + rawReport.endTimestamp ?? + rawReport.end_timestamp ?? + nestedReport.completedAt ?? + nestedReport.completed_at ?? + nestedReport.endTimestamp ?? + nestedReport.end_timestamp ?? null; const completedAt = normalizeTimestamp(rawCompletedAt, fileStats.mtimeMs); const startedAt = normalizeTimestamp( - rawReport.startedAt || - rawReport.started_at || - rawReport.report?.startedAt || - rawReport.report?.started_at || - rawReport.report?.startTimestamp || - rawReport.report?.start_timestamp, + rawReport.startedAt ?? + rawReport.started_at ?? + rawReport.startTimestamp ?? + rawReport.start_timestamp ?? + nestedReport.startedAt ?? + nestedReport.started_at ?? + nestedReport.startTimestamp ?? + nestedReport.start_timestamp, null );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api1.js` around lines 301 - 318, The timestamp lookup for completedAt and startedAt misses flat-report aliases startTimestamp/endTimestamp at the top level; update the rawCompletedAt and startedAt expressions used by normalizeTimestamp to also check rawReport.startTimestamp and rawReport.endTimestamp (and their snake_case forms if needed) in addition to rawReport.report?.startTimestamp/endTimestamp so both nested and flat reports resolve correctly; ensure you still fall back to fileStats.mtimeMs for completedAt and null for startedAt and keep references to normalizeTimestamp, rawReport, completedAt, startedAt, and fileStats.mtimeMs.src/components/market/Market.js (1)
773-780:⚠️ Potential issue | 🔴 CriticalEscape maker addresses before injecting them into HTML.
maker.addressis network-derived offerbook data, but it's interpolated raw intotitle="..."and the inlineonclick. A malformed address containing quotes or markup can break out of the attribute/script context inside the Electron renderer. Set the text/title with DOM APIs and bind the click handler withaddEventListener/data-*instead of string interpolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/market/Market.js` around lines 773 - 780, The maker.address is injected unsafely into HTML attributes and an inline onclick; update the rendering in Market.js to stop string-interpolating maker.address into title and onclick. Instead create the DOM nodes with document.createElement (or your existing renderer utilities), set the tooltip via element.title = formatTorEndpoint(maker.address) and the visible text via element.textContent, store the raw address in a data attribute (e.g., dataset.address) or closure, and attach the click behavior with element.addEventListener('click', () => viewFidelityBond(maker.address)); replace the inline onclick/attribute usage so quotes/markup in maker.address cannot break the renderer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 123-124: Add blank lines immediately before and after the "Using
the AppImage" heading and each surrounding fenced code block so the heading and
code fences are separated by empty lines; update the README's "Using the
AppImage" section (the "Using the AppImage" heading and its opening/closing ```
blocks) and the other similar fenced blocks in that area (the blocks flagged
around the following section) to satisfy markdownlint MD022/MD031.
- Around line 143-145: The README's AppImage extraction instructions still tell
users to run ./TakerApp which mismatches the renamed unpacked binary; update the
steps after "./dist/CoinswapTaker-0.2.1.AppImage --appimage-extract" to either
rename the extracted binary to "coinswap-taker" (matching the rest of the doc)
or directly instruct running "./coinswap-taker" instead of "./TakerApp" so the
command and references (./TakerApp, coinswap-taker) are consistent.
In `@src/components/market/Market.js`:
- Around line 39-53: The formatTorEndpoint helper in Market.js is duplicated in
Swap.js—extract it into a single shared util named formatTorEndpoint (exported
as a named function) and replace the local implementations in both Market.js and
Swap.js with imports of that shared function; ensure the new util exports
formatTorEndpoint, update both components to import { formatTorEndpoint }, and
run the app to confirm behavior is unchanged.
In `@src/components/receive/Receive.js`:
- Around line 43-45: The Receive component currently passes fresh receive
addresses to the external generateQR() service (called where the "Generate
Address" button handler updates elements with classes
generate-text/generate-loading and in the flows around the other generate
calls), which leaks addresses off-box; replace those calls to generateQR() so
the new address is not sent to any third-party URL and instead render the QR
locally in this component using a client-side QR renderer (e.g., a local qrcode
library) when the button with id "generate-new" is clicked and in the other
flows that call generateQR(); update the handlers that toggle elements with
classes "generate-text" and "generate-loading" to call a local
renderQRCode(address) function (or inline local rendering) and remove any
construction of external URLs or calls to generateQR() so addresses are never
transmitted externally.
In `@src/components/swap/Swap.js`:
- Around line 681-687: The tooltip is currently set to selectedMakersText
(abbreviated output from formatTorEndpoint) so hovering doesn't reveal full Tor
addresses; update the code that sets selectedMakersEl.title to use the full
addresses instead (e.g., map getTopCandidateMakers() to maker.address and join
with ', ' or create a selectedMakersFull variable) while keeping
selectedMakersText for the displayed/abbreviated content; reference symbols:
selectedMakersText, selectedMakersEl, getTopCandidateMakers, formatTorEndpoint.
---
Outside diff comments:
In `@api1.js`:
- Around line 301-318: The timestamp lookup for completedAt and startedAt misses
flat-report aliases startTimestamp/endTimestamp at the top level; update the
rawCompletedAt and startedAt expressions used by normalizeTimestamp to also
check rawReport.startTimestamp and rawReport.endTimestamp (and their snake_case
forms if needed) in addition to rawReport.report?.startTimestamp/endTimestamp so
both nested and flat reports resolve correctly; ensure you still fall back to
fileStats.mtimeMs for completedAt and null for startedAt and keep references to
normalizeTimestamp, rawReport, completedAt, startedAt, and fileStats.mtimeMs.
In `@src/components/market/Market.js`:
- Around line 773-780: The maker.address is injected unsafely into HTML
attributes and an inline onclick; update the rendering in Market.js to stop
string-interpolating maker.address into title and onclick. Instead create the
DOM nodes with document.createElement (or your existing renderer utilities), set
the tooltip via element.title = formatTorEndpoint(maker.address) and the visible
text via element.textContent, store the raw address in a data attribute (e.g.,
dataset.address) or closure, and attach the click behavior with
element.addEventListener('click', () => viewFidelityBond(maker.address));
replace the inline onclick/attribute usage so quotes/markup in maker.address
cannot break the renderer.
🪄 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: 1f751e7d-93c5-4f88-8b89-eeaf80dcc22e
📒 Files selected for processing (9)
README.mdapi1.jsdocs/usage.mdsrc/components/market/Market.jssrc/components/receive/Receive.jssrc/components/swap/Swap.jssrc/components/swap/SwapHistory.jssrc/components/swap/SwapReport.jssrc/styles/output.css
Comment on lines
123
to
124
| ### Using the AppImage | ||
| ```bash |
There was a problem hiding this comment.
Add blank lines around this heading and the fenced blocks.
markdownlint is still flagging MD022/MD031 in this section, so the README will keep warning until the heading and each code fence are separated by blank lines.
Also applies to: 133-143
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 123-123: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 124-124: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 123 - 124, Add blank lines immediately before and
after the "Using the AppImage" heading and each surrounding fenced code block so
the heading and code fences are separated by empty lines; update the README's
"Using the AppImage" section (the "Using the AppImage" heading and its
opening/closing ``` blocks) and the other similar fenced blocks in that area
(the blocks flagged around the following section) to satisfy markdownlint
MD022/MD031.
Comment on lines
+143
to
145
| ./dist/CoinswapTaker-0.2.1.AppImage --appimage-extract | ||
| cd squashfs-root | ||
| ./TakerApp |
There was a problem hiding this comment.
Fix the extracted AppImage launch command.
After --appimage-extract, this section still tells users to run ./TakerApp, but the rest of the README has already renamed the unpacked binary to coinswap-taker. Following these steps will fail on the extracted image.
📝 Suggested doc fix
./dist/CoinswapTaker-0.2.1.AppImage --appimage-extract
cd squashfs-root
-./TakerApp
+./coinswap-taker📝 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
| ./dist/CoinswapTaker-0.2.1.AppImage --appimage-extract | |
| cd squashfs-root | |
| ./TakerApp | |
| ./dist/CoinswapTaker-0.2.1.AppImage --appimage-extract | |
| cd squashfs-root | |
| ./coinswap-taker |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 143 - 145, The README's AppImage extraction
instructions still tell users to run ./TakerApp which mismatches the renamed
unpacked binary; update the steps after "./dist/CoinswapTaker-0.2.1.AppImage
--appimage-extract" to either rename the extracted binary to "coinswap-taker"
(matching the rest of the doc) or directly instruct running "./coinswap-taker"
instead of "./TakerApp" so the command and references (./TakerApp,
coinswap-taker) are consistent.
Comment on lines
+39
to
+53
| function formatTorEndpoint(address, start = 14, end = 16) { | ||
| if (!address || typeof address !== 'string') return 'unknown'; | ||
|
|
||
| const separatorIndex = address.lastIndexOf(':'); | ||
| if (separatorIndex === -1) return address; | ||
|
|
||
| const host = address.slice(0, separatorIndex); | ||
| const port = address.slice(separatorIndex + 1); | ||
|
|
||
| if (host.length <= start + end + 3) { | ||
| return `${host}:${port}`; | ||
| } | ||
|
|
||
| return `${host.slice(0, start)}...${host.slice(-end)}:${port}`; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract formatTorEndpoint() into a shared util.
This helper is now identical to the one in src/components/swap/Swap.js, so any truncation tweak has to be kept in sync in two places. Pull it into a shared formatter before the two implementations drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/market/Market.js` around lines 39 - 53, The formatTorEndpoint
helper in Market.js is duplicated in Swap.js—extract it into a single shared
util named formatTorEndpoint (exported as a named function) and replace the
local implementations in both Market.js and Swap.js with imports of that shared
function; ensure the new util exports formatTorEndpoint, update both components
to import { formatTorEndpoint }, and run the app to confirm behavior is
unchanged.
Comment on lines
+43
to
45
| <button id="generate-new" class="w-full bg-[#242d3d] hover:bg-[#2d3748] disabled:bg-gray-700 disabled:cursor-not-allowed text-white font-semibold text-lg py-3 rounded-lg transition-colors border border-gray-700"> | ||
| <span class="generate-text">Generate Address</span> | ||
| <span class="generate-loading hidden"> |
There was a problem hiding this comment.
Don't leak fresh receive addresses to a third-party QR service.
Manual generation is now the primary receive path, and this flow still hands the new address to generateQR(). generateQR() builds an external QR URL, so every fresh receive address is disclosed off-box before the user even copies it. Render the QR locally instead.
Also applies to: 402-405, 419-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/receive/Receive.js` around lines 43 - 45, The Receive
component currently passes fresh receive addresses to the external generateQR()
service (called where the "Generate Address" button handler updates elements
with classes generate-text/generate-loading and in the flows around the other
generate calls), which leaks addresses off-box; replace those calls to
generateQR() so the new address is not sent to any third-party URL and instead
render the QR locally in this component using a client-side QR renderer (e.g., a
local qrcode library) when the button with id "generate-new" is clicked and in
the other flows that call generateQR(); update the handlers that toggle elements
with classes "generate-text" and "generate-loading" to call a local
renderQRCode(address) function (or inline local rendering) and remove any
construction of external URLs or calls to generateQR() so addresses are never
transmitted externally.
Comment on lines
+681
to
+687
| const selectedMakersText = | ||
| getTopCandidateMakers() | ||
| .map((maker) => maker.address) | ||
| .map((maker) => formatTorEndpoint(maker.address)) | ||
| .join(', ') || 'None selected'; | ||
| const selectedMakersEl = content.querySelector('#selected-makers-display'); | ||
| selectedMakersEl.textContent = selectedMakersText; | ||
| selectedMakersEl.title = selectedMakersText; |
There was a problem hiding this comment.
Tooltip shows abbreviated value instead of full addresses.
Setting title to selectedMakersText (the abbreviated text) means the tooltip provides no additional information when hovering. The typical UX pattern is to show the full value in the tooltip when text is truncated, allowing users to see complete Tor addresses on hover.
🔧 Proposed fix to show full addresses in tooltip
const selectedMakersText =
getTopCandidateMakers()
.map((maker) => formatTorEndpoint(maker.address))
.join(', ') || 'None selected';
+ const fullAddressesText =
+ getTopCandidateMakers()
+ .map((maker) => maker.address)
+ .join(', ') || 'None selected';
const selectedMakersEl = content.querySelector('#selected-makers-display');
selectedMakersEl.textContent = selectedMakersText;
- selectedMakersEl.title = selectedMakersText;
+ selectedMakersEl.title = fullAddressesText;📝 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 selectedMakersText = | |
| getTopCandidateMakers() | |
| .map((maker) => maker.address) | |
| .map((maker) => formatTorEndpoint(maker.address)) | |
| .join(', ') || 'None selected'; | |
| const selectedMakersEl = content.querySelector('#selected-makers-display'); | |
| selectedMakersEl.textContent = selectedMakersText; | |
| selectedMakersEl.title = selectedMakersText; | |
| const selectedMakersText = | |
| getTopCandidateMakers() | |
| .map((maker) => formatTorEndpoint(maker.address)) | |
| .join(', ') || 'None selected'; | |
| const fullAddressesText = | |
| getTopCandidateMakers() | |
| .map((maker) => maker.address) | |
| .join(', ') || 'None selected'; | |
| const selectedMakersEl = content.querySelector('#selected-makers-display'); | |
| selectedMakersEl.textContent = selectedMakersText; | |
| selectedMakersEl.title = fullAddressesText; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/swap/Swap.js` around lines 681 - 687, The tooltip is currently
set to selectedMakersText (abbreviated output from formatTorEndpoint) so
hovering doesn't reveal full Tor addresses; update the code that sets
selectedMakersEl.title to use the full addresses instead (e.g., map
getTopCandidateMakers() to maker.address and join with ', ' or create a
selectedMakersFull variable) while keeping selectedMakersText for the
displayed/abbreviated content; reference symbols: selectedMakersText,
selectedMakersEl, getTopCandidateMakers, formatTorEndpoint.
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
Documentation
Bug Fixes
UI/UX Improvements