Conversation
📝 WalkthroughWalkthroughBumps @hoprnet/hopr-sdk prerelease version; UI and state updates: IconButton disables during reloading; Node Info masks/unmasks RPC provider URLs; Tickets page ties reset button to resetTicketStatistics flow with a 2s UI cooldown; OpenSessionModal adds session fields/capabilities and layout changes; store now keeps connected peers as a keyed map and connected peer shape changed; Sessions page renders per-hop PingModal and Hop wrappers; CreateAliasModal prop changed to accept address. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant NodeInfo as Node Info Page
participant Parser as URL Parser
User->>NodeInfo: Load page
NodeInfo->>Parser: Parse provider URL (info?.provider)
Parser-->>NodeInfo: origin/path/search or Error
alt secret present (non-root path or search)
NodeInfo->>NodeInfo: compute masked providerShort or mark invalid
User->>NodeInfo: Click visibility toggle
NodeInfo->>NodeInfo: toggle showWholeProvider (display masked/full)
else no secret
NodeInfo->>NodeInfo: show full provider
end
sequenceDiagram
autonumber
actor User
participant Tickets as Tickets Page
participant Store as Redux Store
participant API as Backend
User->>Tickets: Click "Reset Ticket Statistics"
Tickets->>Store: dispatch resetTicketStatisticsThunk
Store->>API: POST reset ticket stats
API-->>Store: completed
Store-->>Tickets: resettingTicketStatistics true→false
Note over Tickets: UI shows reloading while true, then remains for 2s after completion
Tickets->>Store: dispatch fetchTicketStatistics (refresh)
sequenceDiagram
autonumber
actor User
participant Sessions as Sessions Page
participant Renderer as UI Renderer
participant Ping as PingModal
User->>Sessions: Open Sessions page
Sessions->>Renderer: For each session, parse forward/return path
alt path contains "Hops"
Renderer->>Renderer: render string representation
else JSON path with hops array
Renderer->>Renderer: render each Hop element
Renderer->>Ping: render PingModal on first forward hop
Renderer->>Ping: render PingModal on last return hop
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 2
🧹 Nitpick comments (1)
src/pages/node/info/index.tsx (1)
70-74: Default providerContainsSecret to false to avoid initial flicker.Prevents showing the toggle before parsing runs.
-const [providerContainsSecret, set_providerContainsSecret] = useState(true); +const [providerContainsSecret, set_providerContainsSecret] = useState(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.json(1 hunks)src/future-hopr-lib-components/Button/IconButton.tsx(1 hunks)src/pages/node/info/index.tsx(3 hunks)src/pages/node/tickets.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/node/info/index.tsx (1)
src/future-hopr-lib-components/Button/IconButton.tsx (1)
IconButton(55-83)
src/pages/node/tickets.tsx (1)
src/store/index.ts (1)
useAppSelector(26-26)
🔇 Additional comments (7)
src/future-hopr-lib-components/Button/IconButton.tsx (1)
73-73: Disabling on reloading is correct.Prevents duplicate clicks while animated; pairs well with the Tooltip span wrapper.
Confirm this doesn’t block any “reloading but still clickable” use cases elsewhere.
src/pages/node/tickets.tsx (2)
119-119: Using resettingStats for the button’s reloading is appropriate.With the timer fix, the UX “linger” will be consistent.
26-26: Approve — selector & UI state wiring verified.
Confirmed store.node.resetTicketStatistics.isFetching is referenced in src/pages/node/tickets.tsx (line 26) and matching thunk/state entries exist in src/store/slices/node/actionsAsync.ts, actionsFetching.ts, and initialState.ts.src/pages/node/info/index.tsx (3)
320-326: Copy tweak in tooltip.Small grammar fix.
[raise_nitpick_issue]- title="The RPC provider address your node uses sync" + title="The RPC provider address your node uses to sync"
329-357: Toggle UX reads correctly.Icon/tooltip semantics are clear (Visibility = hide; VisibilityOff = show).
365-368: Provider cell rendering is concise and safe by default.Masked by default; full URL only on explicit toggle; red highlight on faulty RPC is helpful.
package.json (1)
9-9: SDK bump verified — yarn.lock consistent and single resolved version.
package.json and yarn.lock both reference @hoprnet/hopr-sdk@3.0.3-pr.156-20250915124445; no action required.
| useEffect(() => { | ||
| try { | ||
| if (!provider) { | ||
| set_providerShort(''); | ||
| return; | ||
| } | ||
| const providerObject = new URL(provider); | ||
| const providerContainsSecret = providerObject.pathname !== '/' || providerObject.search !== ""; | ||
| set_providerContainsSecret(providerContainsSecret); | ||
| const providerShort = providerContainsSecret ? providerObject.origin + '/************' : provider; | ||
| set_providerShort(providerShort || ''); | ||
| } catch (e) { | ||
| console.error('Error parsing provider URL', e); | ||
| set_providerShort('***Invalid URL***'); | ||
| } | ||
| }, [provider]); | ||
|
|
There was a problem hiding this comment.
Harden provider parsing; avoid shadowing; auto-hide on change.
- Don’t shadow state with a local providerContainsSecret.
- Reset showWholeProvider on provider change to avoid exposing a new URL.
- Treat userinfo and query as secrets; fall back cleanly on errors/empty provider.
Apply:
useEffect(() => {
- try {
- if (!provider) {
- set_providerShort('');
- return;
- }
- const providerObject = new URL(provider);
- const providerContainsSecret = providerObject.pathname !== '/' || providerObject.search !== "";
- set_providerContainsSecret(providerContainsSecret);
- const providerShort = providerContainsSecret ? providerObject.origin + '/************' : provider;
- set_providerShort(providerShort || '');
- } catch (e) {
- console.error('Error parsing provider URL', e);
- set_providerShort('***Invalid URL***');
- }
+ // Always hide full URL when provider changes
+ set_showWholeProvider(false);
+ try {
+ if (!provider) {
+ set_providerContainsSecret(false);
+ set_providerShort('');
+ return;
+ }
+ const u = new URL(provider);
+ const containsSecret =
+ u.username !== '' ||
+ u.password !== '' ||
+ u.pathname !== '/' ||
+ u.search !== '';
+ set_providerContainsSecret(containsSecret);
+ set_providerShort(containsSecret ? `${u.origin}/************` : provider);
+ } catch (e) {
+ console.error('Error parsing provider URL', e);
+ set_providerContainsSecret(false);
+ set_providerShort('***Invalid URL***');
+ }
}, [provider]);📝 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.
| useEffect(() => { | |
| try { | |
| if (!provider) { | |
| set_providerShort(''); | |
| return; | |
| } | |
| const providerObject = new URL(provider); | |
| const providerContainsSecret = providerObject.pathname !== '/' || providerObject.search !== ""; | |
| set_providerContainsSecret(providerContainsSecret); | |
| const providerShort = providerContainsSecret ? providerObject.origin + '/************' : provider; | |
| set_providerShort(providerShort || ''); | |
| } catch (e) { | |
| console.error('Error parsing provider URL', e); | |
| set_providerShort('***Invalid URL***'); | |
| } | |
| }, [provider]); | |
| useEffect(() => { | |
| // Always hide full URL when provider changes | |
| set_showWholeProvider(false); | |
| try { | |
| if (!provider) { | |
| set_providerContainsSecret(false); | |
| set_providerShort(''); | |
| return; | |
| } | |
| const u = new URL(provider); | |
| const containsSecret = | |
| u.username !== '' || | |
| u.password !== '' || | |
| u.pathname !== '/' || | |
| u.search !== ''; | |
| set_providerContainsSecret(containsSecret); | |
| set_providerShort(containsSecret ? `${u.origin}/************` : provider); | |
| } catch (e) { | |
| console.error('Error parsing provider URL', e); | |
| set_providerContainsSecret(false); | |
| set_providerShort('***Invalid URL***'); | |
| } | |
| }, [provider]); |
🤖 Prompt for AI Agents
In src/pages/node/info/index.tsx around lines 79 to 95, the effect that parses
the provider URL shadows component state, doesn't reset the showWholeProvider
flag when provider changes, and doesn't treat URL userinfo or query as secrets;
update the effect to (1) avoid declaring a local providerContainsSecret variable
that shadows state—use the state setter directly, (2) always reset
showWholeProvider to false at the start of the effect when provider changes so a
new provider isn't accidentally revealed, (3) consider pathname !== '/' OR
presence of userinfo OR non-empty search/query as secrets, and (4) on empty
provider or any parsing error set providerShort to a redacted placeholder (e.g.
'***Invalid URL***' or empty) and ensure providerContainsSecret is set false;
keep console.error for debugging.
| useEffect(() => { | ||
| if (resettingTicketStatistics) { | ||
| set_resettingStats(true); | ||
| } else { | ||
| setTimeout(() => { | ||
| set_resettingStats(false); | ||
| }, 2000); | ||
| } | ||
| }, [resettingTicketStatistics]); |
There was a problem hiding this comment.
Cleanup timer to avoid stale updates and memory leaks.
Currently, multiple quick toggles can flip reloading off prematurely; also setState may fire after unmount.
Apply:
- useEffect(() => {
- if (resettingTicketStatistics) {
- set_resettingStats(true);
- } else {
- setTimeout(() => {
- set_resettingStats(false);
- }, 2000);
- }
- }, [resettingTicketStatistics]);
+ useEffect(() => {
+ let t: number | undefined;
+ if (resettingTicketStatistics) {
+ set_resettingStats(true);
+ } else {
+ t = window.setTimeout(() => set_resettingStats(false), 2000);
+ }
+ return () => {
+ if (t) window.clearTimeout(t);
+ };
+ }, [resettingTicketStatistics]);📝 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.
| useEffect(() => { | |
| if (resettingTicketStatistics) { | |
| set_resettingStats(true); | |
| } else { | |
| setTimeout(() => { | |
| set_resettingStats(false); | |
| }, 2000); | |
| } | |
| }, [resettingTicketStatistics]); | |
| useEffect(() => { | |
| let t: number | undefined; | |
| if (resettingTicketStatistics) { | |
| set_resettingStats(true); | |
| } else { | |
| t = window.setTimeout(() => set_resettingStats(false), 2000); | |
| } | |
| return () => { | |
| if (t) window.clearTimeout(t); | |
| }; | |
| }, [resettingTicketStatistics]); |
🤖 Prompt for AI Agents
In src/pages/node/tickets.tsx around lines 38 to 46, the useEffect creates a
timeout without clearing it, which can cause stale updates or setState after
unmount and lets rapid toggles flip resettingStats off prematurely; fix by
storing the timeout id in a ref, clear any existing timeout before creating a
new one, and return a cleanup that clears the timeout and marks the component as
unmounted (or checks the ref) so set_resettingStats is not called after unmount.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/store/slices/node/initialState.ts (1)
162-166: Rename connected index signature to peerAddress for consistencyconnected in src/store/slices/node/initialState.ts is keyed by address but the index signature is still named
peerId; change it topeerAddressto match other maps in this file.- [peerId: string]: { + [peerAddress: string]: { address: string; quality: number; multiaddr: string | null;This is a local/type-name rename and safe; other occurrences in the repo already use
peerAddress.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Modal/node/OpenSessionModal.tsx(0 hunks)src/store/slices/node/actionsAsync.ts(1 hunks)src/store/slices/node/initialState.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/Modal/node/OpenSessionModal.tsx
| action.payload?.connected.forEach((peer) => { | ||
| state.peers.parsed.connected[peer.address] = peer; | ||
| }); |
There was a problem hiding this comment.
Fix stale peers: clear the map before repopulating
Without resetting, disconnected peers linger in parsed.connected. Rebuild the map atomically from payload.
Apply:
- action.payload?.connected.forEach((peer) => {
- state.peers.parsed.connected[peer.address] = peer;
- });
+ state.peers.parsed.connected = Object.fromEntries(
+ (action.payload?.connected ?? []).map((peer) => [peer.address, peer]),
+ );📝 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.
| action.payload?.connected.forEach((peer) => { | |
| state.peers.parsed.connected[peer.address] = peer; | |
| }); | |
| state.peers.parsed.connected = Object.fromEntries( | |
| (action.payload?.connected ?? []).map((peer) => [peer.address, peer]), | |
| ); |
🤖 Prompt for AI Agents
In src/store/slices/node/actionsAsync.ts around lines 1151 to 1153, the code
appends peers to state.peers.parsed.connected without clearing it first, causing
stale/disconnected peers to persist; replace the incremental population with an
atomic rebuild: create a fresh map/object from action.payload?.connected (safely
handling undefined) and then assign that new object to
state.peers.parsed.connected (or replace the map in one assignment) so the map
is cleared and repopulated atomically from the payload.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/node/channelsOutgoing.tsx (1)
201-211: Avoid falsy checks that drop valid rows (e.g., zero balance) and guard missing entriesUse nullish checks and avoid repeated indexing.
- const id = channel.id; - if ( - !channelsOutgoingObject[id].peerAddress || - !channelsOutgoingObject[id].balance || - !channelsOutgoingObject[id].status - ) - return; - - const peerAddress = channelsOutgoingObject[id].peerAddress; - if (!peerAddress) return; + const id = channel.id; + const channelObj = channelsOutgoingObject?.[id]; + if (!channelObj) return; + const { peerAddress, balance, status } = channelObj; + if (peerAddress == null || balance == null || status == null) return;Optional follow-up: where used later, prefer
channelObj?.isClosingoverchannelsOutgoingObject[id]?.isClosingfor consistency.src/components/Modal/node/OpenSessionModal.tsx (1)
253-254: Bug: returnPath uses forward hops countWhen
sendReturnMode === 'numberOfHops', you setreturnPath.HopsfromnumberOfForwardHops. Should benumberOfReturnHops. This will generate incorrect routes.Apply this diff:
- sessionPayload.returnPath = { - Hops: numberOfForwardHops, - }; + sessionPayload.returnPath = { + Hops: numberOfReturnHops, + };
🧹 Nitpick comments (6)
src/pages/node/channelsOutgoing.tsx (2)
26-26: Fix double slash in import pathSome tooling (e.g., eslint-plugin-import) flags this; normalize the path.
-import { CreateAliasModal } from '../../components/Modal/node//AddAliasModal'; +import { CreateAliasModal } from '../../components/Modal/node/AddAliasModal';
316-316: Verify default sort key
orderByDefault="number"doesn’t match any header keys ('id','node', etc.). If the intent is the row number column, use'id'.- orderByDefault="number" + orderByDefault="id"src/components/Modal/node/OpenSessionModal.tsx (4)
120-126: State defaults OK; consider naming and validationDefaults look sane. Minor:
maxSurbUpstreamis a rate; consider naming likemaxSurbUpstreamKBpsto make units explicit, and clamp values at input or before dispatch.
414-451: Destination + Max client: type duplication and input hygiene
TextFieldfor “Max client” sets bothtype='number'andinputProps={{ type: 'number' }}. Keep one to avoid inconsistencies.- Consider clamping to [1, 10000] on change to prevent transient invalid state.
- <TextField + <TextField label="Max client" value={maxClientSessions} onChange={(event) => { - set_maxClientSessions(Number(event?.target?.value)); + const v = Number(event?.target?.value); + set_maxClientSessions(Number.isFinite(v) ? Math.min(10000, Math.max(1, v)) : 1); }} - inputProps={{ - type: 'number', + inputProps={{ min: 1, max: 10000, step: 1, }} - type='number' style={{ width: '97px', }} />
459-487: Max SURB upstream: unit in UI mismatches payloadUI shows “kb/s” while buffer uses “kB”. If you standardize to “kB/s” in payload (see earlier comment), also update the adornment here.
- InputProps={{ - endAdornment: <InputAdornment position="end">kb/s</InputAdornment>, - }} + InputProps={{ + endAdornment: <InputAdornment position="end">kB/s</InputAdornment>, + }}Also applies to: 481-482
414-433: Autocomplete getOptionLabel safety
aliases[address]assumesaddressis a key inaliases. Ifaliasesis keyed differently (e.g., alias->address), this can renderundefined (...). Consider optional chaining or a mapping helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
package.json(1 hunks)src/components/Modal/node/OpenSessionModal.tsx(7 hunks)src/pages/node/channelsOutgoing.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/node/channelsOutgoing.tsx (1)
src/components/Modal/node/AddAliasModal.tsx (1)
CreateAliasModal(21-184)
src/components/Modal/node/OpenSessionModal.tsx (2)
src/store/index.ts (1)
useAppSelector(26-26)src/future-hopr-lib-components/Modal/styled.tsx (1)
TopBar(15-20)
🔇 Additional comments (8)
src/pages/node/channelsOutgoing.tsx (1)
236-236: CreateAliasModal API alignment looks goodPassing
address={peerAddress}matches the updated modal API and is safe here since you guard for a definedpeerAddressabove.Minor: confirm that
peerAddressis always an Ethereum‑style address (whatisAddressvalidates in the modal) and never a peerId in this view.src/components/Modal/node/OpenSessionModal.tsx (7)
132-134: Sensible defaults for listener and targetUsing loopback defaults is a good DX choice.
395-395: Confirm SDialog maxWidth typing
maxWidth={'750px'}is not a valid MUIDialogprop (expects breakpoint enum) unlessSDialogre-maps it to a styled prop. Please confirmSDialogsupports pixel strings; otherwise, set width viaPaperProps.sx={{ maxWidth: 750 }}or container styles.[suggest_minor_refactor]
398-398: Copy: “Open session listener”Title change reads clearer.
383-384: Tooltip tweak LGTMThe tooltip ending with “session listener” matches the title.
583-591: NoRateControl toggleLooks straightforward and independent.
237-239: ```shell
#!/bin/bash
set -euo pipefailecho "=== rg: maxSurbUpstream ==="
rg -n --hidden -S 'maxSurbUpstream' || trueecho "=== rg: maxClientSessions ==="
rg -n --hidden -S 'maxClientSessions' || trueecho "=== rg: responseBuffer ==="
rg -n --hidden -S 'responseBuffer' || trueecho "=== rg: unit strings (kb/s kB/s kB kb) ==="
rg -n --hidden -S 'kb/s|kB/s|\bkB\b|\bkb\b' || trueecho "=== show OpenSessionModal.tsx (if exists) lines 1-400 ==="
if [ -f src/components/Modal/node/OpenSessionModal.tsx ]; then
sed -n '1,400p' src/components/Modal/node/OpenSessionModal.tsx
else
echo "File not found: src/components/Modal/node/OpenSessionModal.tsx"
fiecho "=== find types/interfaces mentioning Session or SessionConfig ==="
rg -n --hidden -S 'interface .*Session|type .*Session|SessionConfig|OpenSession|StartSession' -g '!node_modules' || trueecho "=== find calls that might send this payload to SDK/backend ==="
rg -n --hidden -S 'createSession|openSession|startSession|start_node_session|sendSession|startNode' -g '!node_modules' || trueecho "=== search for SDK/client imports/usages ==="
rg -n --hidden -S 'from .*sdk|from .*client|new .*Client|createClient(|getClient(' -g '!node_modules' || true--- `224-239`: **Verify SDK payload contract for @hoprnet/hopr-sdk 3.0.3-pr.156** - Provide the exact OpenSessionPayloadType (TS interface or compiled .d.ts) from your installed package or a link to the exact repo/tag/file for 3.0.3-pr.156. - Confirm allowed capability literals (expected: Retransmission, RetransmissionAckOnly, Segmentation, NoDelay, NoRateControl) and whether `capabilities` is string[] / enum. - Confirm expected types/units for `responseBuffer` and `maxSurbUpstream` (number vs string, and whether unit suffixes like "kB" / "kb/s" are required). - Confirm `target` shape `{ Plain: string }` is still valid. Affects: src/components/Modal/node/OpenSessionModal.tsx lines 224-239 and 262-276. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| if(!retransmission) { | ||
| set_segmentation(true); | ||
| } | ||
| return !retransmission; |
There was a problem hiding this comment.
Make Retransmission and RetransmissionAckOnly mutually exclusive; keep Segmentation consistent
Likely the SDK treats Retransmission and RetransmissionAckOnly as exclusive. Right now both can be enabled and both are pushed to capabilities. Also, you set Segmentation true when enabling any of these, but it may remain true after disabling them. Enforce exclusivity and keep Segmentation consistent.
<FormControlLabel
control={<Checkbox checked={retransmission} />}
label="Retransmission"
onChange={() => {
- set_retransmission((retransmission) => {
- if(!retransmission) {
- set_segmentation(true);
- }
- return !retransmission;
- });
+ set_retransmission((prev) => {
+ const next = !prev;
+ if (next) {
+ set_retransmissionAckOnly(false);
+ set_segmentation(true);
+ }
+ return next;
+ });
}}
/>
<FormControlLabel
control={<Checkbox checked={retransmissionAckOnly} />}
label="RetransmissionAckOnly"
onChange={() => {
- set_retransmissionAckOnly((retransmissionAckOnly) => {
- if(!retransmissionAckOnly) {
- set_segmentation(true);
- }
- return !retransmissionAckOnly;
- });
+ set_retransmissionAckOnly((prev) => {
+ const next = !prev;
+ if (next) {
+ set_retransmission(false);
+ set_segmentation(true);
+ }
+ return next;
+ });
}}
/>
<FormControlLabel
control={<Checkbox checked={segmentation} />}
label="Segmentation"
disabled={noDelay || retransmission || retransmissionAckOnly}
title={
retransmission ?
'Segmentation is required when Retransmission is enabled' :
retransmissionAckOnly ?
'Segmentation is required when RetransmissionAckOnly is enabled' :
noDelay ?
'Segmentation is required when NoDelay is enabled' :
''}
onChange={() => {
set_segmentation((segmentation) => {
return !segmentation;
});
}}
/>
<FormControlLabel
control={<Checkbox checked={noDelay} />}
label="NoDelay"
onChange={() => {
set_noDelay((noDelay) => {
if(!noDelay) {
set_segmentation(true);
}
return !noDelay;
});
}}
/>Also consider deriving an effectiveSegmentation = segmentation || noDelay || retransmission || retransmissionAckOnly and pushing Segmentation based on that, to avoid stale state.
Also applies to: 540-551, 555-563, 571-581
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Modal/node/OpenSessionModal.tsx (1)
252-255: Return-path bug: uses numberOfForwardHops instead of numberOfReturnHopsThis will send the wrong hop count for return paths.
- if (sendReturnMode === 'numberOfHops') { - sessionPayload.returnPath = { - Hops: numberOfForwardHops, - }; - } + if (sendReturnMode === 'numberOfHops') { + sessionPayload.returnPath = { + Hops: numberOfReturnHops, + }; + }src/pages/node/sessions.tsx (1)
165-187: Fix search/copy for sessions "Path" column — TablePro only handles string cell valuesTablePro's search only matches when a cell is a string (filterData checks typeof === 'string') and copy-on-double-click only triggers for string values; sessions.tsx sets header.key='path' with search: true, copy: true while parsedTableData.path is JSX — search and copy will not work. (Files: src/pages/node/sessions.tsx; behavior in src/future-hopr-lib-components/Table/table-pro.tsx — filterData & onDoubleClick.)
Actions (choose one):
- Disable search/copy on the 'path' header in src/pages/node/sessions.tsx.
- Or add a plain-text fallback (e.g., parsedTableData.pathText) and either update TablePro to prefer a header.searchKey/header.copyKey fallback or wire header to the text key while keeping a separate JSX display field (changes in sessions.tsx and table-pro.tsx).
♻️ Duplicate comments (1)
src/components/Modal/node/OpenSessionModal.tsx (1)
541-551: Make Retransmission and RetransmissionAckOnly mutually exclusive; keep Segmentation consistentCurrent UI allows enabling both; also Segmentation state can drift. Enforce exclusivity and force Segmentation when either is enabled. (Same concern raised previously.)
<FormControlLabel control={<Checkbox checked={retransmission} />} label="Retransmission" onChange={() => { - set_retransmission((retransmission) => { - if (!retransmission) { - set_segmentation(true); - } - return !retransmission; - }); + set_retransmission((prev) => { + const next = !prev; + if (next) { + set_retransmissionAckOnly(false); + set_segmentation(true); + } + return next; + }); }} /> <FormControlLabel control={<Checkbox checked={retransmissionAckOnly} />} label="RetransmissionAckOnly" onChange={() => { - set_retransmissionAckOnly((retransmissionAckOnly) => { - if (!retransmissionAckOnly) { - set_segmentation(true); - } - return !retransmissionAckOnly; - }); + set_retransmissionAckOnly((prev) => { + const next = !prev; + if (next) { + set_retransmission(false); + set_segmentation(true); + } + return next; + }); }} /> <FormControlLabel control={<Checkbox checked={segmentation} />} label="Segmentation" - disabled={noDelay || retransmission || retransmissionAckOnly} + disabled={noDelay || retransmission || retransmissionAckOnly} title={ retransmission ? 'Segmentation is required when Retransmission is enabled' : retransmissionAckOnly ? 'Segmentation is required when RetransmissionAckOnly is enabled' : noDelay ? 'Segmentation is required when NoDelay is enabled' : '' }Also applies to: 571-582, 555-564
🧹 Nitpick comments (4)
src/pages/node/sessions.tsx (3)
24-29: Avoid fixed height on Hop to prevent clippingLong addresses or different font metrics can get clipped. Prefer min-height/line-height and add small gap.
-const Hop = styled.div` - display: inline-flex; - align-items: center; - margin: 2px; - height: 18px; -`; +const Hop = styled.div` + display: inline-flex; + align-items: center; + margin: 2px; + min-height: 18px; + line-height: 18px; + gap: 4px; +`;
169-174: Don’t use JSON.stringify to detect shape; check properties insteadString-searching for "Hops" is brittle and slower. Check for the key directly.
- {JSON.stringify(session.forwardPath).includes('Hops') ? ( + {(session?.forwardPath as any)?.Hops !== undefined ? ( JSON.stringify(session.forwardPath) .replace(/{|}|\[|\]|"/g, '') .replace('IntermediatePath:', 'IntermediatePath:\n') .replace(/,/g, ' ') ) : (- {JSON.stringify(session.returnPath).includes('Hops') ? ( + {(session?.returnPath as any)?.Hops !== undefined ? ( JSON.stringify(session.returnPath) .replace(/{|}|\[|\]|"/g, '') .replace('IntermediatePath:', 'IntermediatePath:\n') .replace(/,/g, ' ') ) : (Also applies to: 192-197
176-183: Use more stable React keys for hopsIndex-only keys can cause incorrect reuse on reordering. Include hop value.
- key={`hop-f-${i}`} + key={`hop-f-${i}-${hop}`}- key={`hop-r-${i}`} + key={`hop-r-${i}-${hop}`}Also applies to: 199-206
src/components/Modal/node/OpenSessionModal.tsx (1)
459-467: Add basic listenHost validation (host:port)A malformed listenHost will bounce at the API. Add a lightweight check and surface inline error.
- <TextField + <TextField label="Listen host" placeholder={'127.0.0.1:10000'} value={listenHost} onChange={(event) => { set_listenHost(event?.target?.value); }} + error={!!listenHost && !/^[\[\]a-zA-Z0-9._:-]+:\d{1,5}$/.test(listenHost)} + helperText={ + !!listenHost && !/^[\[\]a-zA-Z0-9._:-]+:\d{1,5}$/.test(listenHost) + ? 'Expected host:port (IPv4/IPv6-with-brackets supported)' + : undefined + } fullWidth />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Modal/node/OpenSessionModal.tsx(8 hunks)src/pages/node/info/index.tsx(3 hunks)src/pages/node/sessions.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/node/info/index.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/node/sessions.tsx (1)
src/components/Modal/node/PingModal.tsx (1)
PingModal(22-186)
src/components/Modal/node/OpenSessionModal.tsx (2)
src/store/index.ts (1)
useAppSelector(26-26)src/future-hopr-lib-components/Modal/styled.tsx (1)
TopBar(15-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Publish
🔇 Additional comments (4)
src/pages/node/sessions.tsx (1)
18-18: Ping controls on endpoint hops — LGTMUsing PingModal at the first forward hop and last return hop is a sensible UX compromise.
Also applies to: 182-183, 205-206
src/components/Modal/node/OpenSessionModal.tsx (3)
344-347: Closing modal no longer resets key fields — confirm UXWith these resets commented out, subsequent openings keep prior values. If intentional, ignore; otherwise restore resets or add an explicit “Reset” action.
237-239: Confirm SDK contract for maxSurbUpstream and maxClientSessions — provide SDK version/typesI need the exact @hoprnet/hopr-sdk prerelease (package.json version or commit) or the SDK's OpenSessionPayloadType definition so I can verify whether maxSurbUpstream should be a number (bytes/sec or bits/sec) or a unit-suffixed string (kB/s vs kb/s) and the expected type/bounds for maxClientSessions.
Applies to: src/components/Modal/node/OpenSessionModal.tsx lines 237–239 and 120–122.
584-592: Verify that "NoRateControl" is a supported session capability in the SDK / hoprd APIpackage.json pins @hoprnet/hopr-sdk@3.0.3-pr.156-20250916114518 and this component pushes "NoRateControl" into sessionPayload.capabilities (src/components/Modal/node/OpenSessionModal.tsx); I couldn't find "NoRateControl" in the SDK docs/repo — confirm the exact capability string accepted by the SDK or hoprd OpenAPI and update the payload/label if it differs. (github.com)
1 participant
Changelog:
Summary by CodeRabbit
New Features
Bug Fixes
Chores