Improve P2P stats peer locations and Kubo display#1139
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds peer geolocation with online GeoIP lookups and offline fallback, introduces peer role indicators (seeder/leecher), and brings RPC-based peer statistics to browser environments by implementing a new ChangesPeer Geolocation and Role-Based Stats Display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/components/settings-modal/p2p-stats-settings/__tests__/p2p-stats-settings.test.tsx`:
- Around line 243-248: The test currently grabs the first SVG rect via
container.querySelector('svg rect'), which is order-dependent; instead locate
the specific marker by its title and assert on that rect. Replace the use of
container.querySelector('svg rect') (the marker variable) with logic that finds
the SVG rect whose sibling <title> text equals the intended marker label (e.g.,
querySelectorAll('svg g').find(g => g.querySelector('title')?.textContent ===
'EXPECTED_TITLE') and then select g.querySelector('rect')), and update the
coordinate assertions to use that rect; apply the same change to the other
occurrence around lines 313-320. Ensure the code references the existing marker
variable name and uses getAttribute('x'), getAttribute('y'), 'width', and
'height' as before.
In
`@src/components/settings-modal/p2p-stats-settings/p2p-stats-settings.module.css`:
- Around line 122-124: The CSS rule for the selector
.connectionRole[data-peer-role='leecher'] uses a hardcoded "red" value; replace
that literal with the appropriate theme semantic color token (e.g.,
--color-peer-leecher or an existing semantic token like
--color-error/--color-destructive) and reference that token in all places that
style the leecher role (including the other occurrence noted at lines ~198-200).
If no suitable token exists, add a new semantic token to the theme tokens file
(name it consistently, e.g., --color-peer-leecher) and use that token in both
selectors so the color follows theme palette and contrast rules.
In `@src/components/settings-modal/p2p-stats-settings/p2p-stats-settings.tsx`:
- Around line 568-575: The current code calls fetch against DNS_JSON_URL with
normalizedHostname, leaking user-configured hostnames to a third-party resolver;
change the logic in the block that builds value so you only perform the external
fetch when explicitly allowed: first, if normalizedHostname is already a literal
IP (use existing isIpv4Address / isIpv6Address check) use that directly and skip
any fetch; otherwise do not call fetch unless an explicit opt-in boolean (e.g.,
allowExternalDnsLookup) is true—if you add that prop/state, check it before
invoking fetch(DNS_JSON_URL...) and only then use
isRecord/getStringValue/isLikelyPublicIp on the response; ensure default
behavior is to avoid any external lookup.
- Around line 568-569: The current DNS lookup uses only type=A (IPv4) which
fails for IPv6-only hosts; update the logic around the fetch to also query
type=AAAA for IPv6 addresses: either perform a second fetch to
`${DNS_JSON_URL}?name=${encodeURIComponent(normalizedHostname)}&type=AAAA` when
the A query returns no usable answers, or perform both queries and prefer AAAA
if present; then merge/choose results accordingly where the code inspects the
`response` (the object returned from the fetch call) and its `Answer`/`answers`
field so endpoint availability checks accept IPv6 addresses too. Ensure you
reuse `DNS_JSON_URL` and `normalizedHostname` and preserve existing
error/timeout handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: edf9488a-1260-43cc-a57a-74d93a1fe1a3
📒 Files selected for processing (9)
src/components/settings-modal/p2p-stats-settings/__tests__/p2p-stats-settings.test.tsxsrc/components/settings-modal/p2p-stats-settings/__tests__/peer-world-map.test.tsxsrc/components/settings-modal/p2p-stats-settings/p2p-stats-settings.module.csssrc/components/settings-modal/p2p-stats-settings/p2p-stats-settings.tsxsrc/components/settings-modal/p2p-stats-settings/peer-world-map.tsxsrc/lib/__tests__/p2p-runtime.test.tssrc/lib/__tests__/peer-geo.test.tssrc/lib/p2p-runtime.tssrc/lib/peer-geo.ts
| .connectionRole[data-peer-role='leecher'] { | ||
| color: red; | ||
| } |
There was a problem hiding this comment.
Use theme token(s) instead of hardcoded red for leecher styling.
Hardcoded red bypasses theme palette and can break contrast/consistency across themes. Use an existing semantic color variable (or add one in theme tokens) and reference it in both selectors.
As per coding guidelines, "Preserve the old-school imageboard aesthetic ... and the existing theme palette" and "Do not introduce ... new accent/marketing colors."
Also applies to: 198-200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/components/settings-modal/p2p-stats-settings/p2p-stats-settings.module.css`
around lines 122 - 124, The CSS rule for the selector
.connectionRole[data-peer-role='leecher'] uses a hardcoded "red" value; replace
that literal with the appropriate theme semantic color token (e.g.,
--color-peer-leecher or an existing semantic token like
--color-error/--color-destructive) and reference that token in all places that
style the leecher role (including the other occurrence noted at lines ~198-200).
If no suitable token exists, add a new semantic token to the theme tokens file
(name it consistently, e.g., --color-peer-leecher) and use that token in both
selectors so the color follows theme palette and contrast rules.
| const response = await fetch(`${DNS_JSON_URL}?name=${encodeURIComponent(normalizedHostname)}&type=A`, { | ||
| headers: { accept: 'application/dns-json' }, |
There was a problem hiding this comment.
Add IPv6 hostname resolution support (AAAA), not only A records.
type=A fails for IPv6-only RPC hosts, making endpoint resolution incorrectly return unavailable in those setups.
Suggested change
- const response = await fetch(`${DNS_JSON_URL}?name=${encodeURIComponent(normalizedHostname)}&type=A`, {
+ const [aResponse, aaaaResponse] = await Promise.all([
+ fetch(`${DNS_JSON_URL}?name=${encodeURIComponent(normalizedHostname)}&type=A`, {
+ headers: { accept: 'application/dns-json' },
+ signal,
+ }),
+ fetch(`${DNS_JSON_URL}?name=${encodeURIComponent(normalizedHostname)}&type=AAAA`, {
+ headers: { accept: 'application/dns-json' },
+ signal,
+ }),
+ ]);
- headers: { accept: 'application/dns-json' },
- signal,
- });
- const data = response.ok ? await response.json() : undefined;
- const answers = isRecord(data) && Array.isArray(data.Answer) ? data.Answer : [];
+ const aData = aResponse.ok ? await aResponse.json() : undefined;
+ const aaaaData = aaaaResponse.ok ? await aaaaResponse.json() : undefined;
+ const answers = [
+ ...(isRecord(aData) && Array.isArray(aData.Answer) ? aData.Answer : []),
+ ...(isRecord(aaaaData) && Array.isArray(aaaaData.Answer) ? aaaaData.Answer : []),
+ ];📝 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.
| const response = await fetch(`${DNS_JSON_URL}?name=${encodeURIComponent(normalizedHostname)}&type=A`, { | |
| headers: { accept: 'application/dns-json' }, | |
| const [aResponse, aaaaResponse] = await Promise.all([ | |
| fetch(`${DNS_JSON_URL}?name=${encodeURIComponent(normalizedHostname)}&type=A`, { | |
| headers: { accept: 'application/dns-json' }, | |
| signal, | |
| }), | |
| fetch(`${DNS_JSON_URL}?name=${encodeURIComponent(normalizedHostname)}&type=AAAA`, { | |
| headers: { accept: 'application/dns-json' }, | |
| signal, | |
| }), | |
| ]); | |
| const aData = aResponse.ok ? await aResponse.json() : undefined; | |
| const aaaaData = aaaaResponse.ok ? await aaaaResponse.json() : undefined; | |
| const answers = [ | |
| ...(isRecord(aData) && Array.isArray(aData.Answer) ? aData.Answer : []), | |
| ...(isRecord(aaaaData) && Array.isArray(aaaaData.Answer) ? aaaaData.Answer : []), | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/settings-modal/p2p-stats-settings/p2p-stats-settings.tsx`
around lines 568 - 569, The current DNS lookup uses only type=A (IPv4) which
fails for IPv6-only hosts; update the logic around the fetch to also query
type=AAAA for IPv6 addresses: either perform a second fetch to
`${DNS_JSON_URL}?name=${encodeURIComponent(normalizedHostname)}&type=AAAA` when
the A query returns no usable answers, or perform both queries and prefer AAAA
if present; then merge/choose results accordingly where the code inspects the
`response` (the object returned from the fetch call) and its `Answer`/`answers`
field so endpoint availability checks accept IPv6 addresses too. Ensure you
reuse `DNS_JSON_URL` and `normalizedHostname` and preserve existing
error/timeout handling.
|
Addressed the valid CodeRabbit findings in the latest commit: marker tests now select markers by title instead of SVG order, and full-node RPC hostnames are no longer resolved through an external DNS service. Remaining bot notes are non-blocking: leecher markers intentionally use browser-default CSS red per the product requirement, and the IPv6 AAAA DNS note is no longer applicable because hostname DNS lookup was removed; literal public IPv6 hosts still go through the existing IP path.\n\nLocal verification after the review fix:\n- corepack yarn test src/components/settings-modal/p2p-stats-settings/tests/p2p-stats-settings.test.tsx --run\n- corepack yarn test --run\n- corepack yarn type-check\n- corepack yarn lint\n- corepack yarn build\n- corepack yarn doctor\n- git diff --check |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ea1924d. Configure here.
| const statsAddresses = [...getAddressListFromRecord(identity), ...getAddressListFromRecord(stats)]; | ||
| const peerRecords = rpcPeers ?? getFirstNestedValue(stats, [['peers'], ['Peers'], ['connectedPeers'], ['connections']]); | ||
| const connectedPeers = await resolveConnectedPeerLocations(getConnectedPeersRowFromRecords(peerRecords), signal); | ||
| const nodeEndpoint = statsAddresses.length ? await resolveOwnEndpoint(statsAddresses, signal) : await getFullNodeRpcEndpoint(account, signal); |
There was a problem hiding this comment.
Full-node Your IP wrong
Medium Severity
For browser full-node RPC stats, Your IP is chosen with statsAddresses.length instead of whether any identity address is a public IP. When RPC identity only lists private or non-routable multiaddrs, the UI still calls resolveOwnEndpoint, which falls back to the browser’s public endpoint rather than the RPC host from getFullNodeRpcEndpoint.
Reviewed by Cursor Bugbot for commit ea1924d. Configure here.
1 participant
Summary
Verification
Note
Medium Risk
Adds a new runtime mode and expands P2P stats to call an external GeoIP service for peer IPs, which can affect privacy, network behavior, and UI correctness. Changes also rework Kubo/full-node stats parsing and display, increasing the chance of regressions across runtimes.
Overview
Improves the P2P stats panel by resolving city-level peer and own-node map markers via external GeoIP lookup with caching and an offline fallback, and by styling leecher vs seeder roles in both the peer list and world map.
Adds a browser
full-node-rpcruntime mode and new RPC-backed stats path that parsesgetStats/getPeers, populates the same connected-peer/map model, and avoids resolving hostname RPC endpoints through external DNS.Simplifies Electron Kubo stats output (now
Seeding+Kubo RPC,Data received/sent) and avoids showing relay/p2p-circuitaddresses asYour IP. Test coverage is expanded substantially for map placement, role styling, RPC mode behavior, and GeoIP caching/fallbacks.Reviewed by Cursor Bugbot for commit ea1924d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
UI / Style
Tests