Conversation
📝 WalkthroughWalkthroughThis PR updates Node.js versions across CI/Docker, pins Node 22.x in release workflow, comments out a Yarn registry, bumps @hoprnet/hopr-sdk, adds a new “corrupted channels” thunk/state with related dispatches, adjusts Info page UI and data flow to use it, enhances strategies YAML export, and tweaks Sessions MTU mapping. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant UI as ConnectNodeModal
participant R as Router
participant S as Store/Redux
participant API as Node API
U->>UI: Submit endpoint/token
UI->>S: dispatch(getAddressesThunk)
S->>API: getAddresses
API-->>S: addresses
S-->>UI: addresses loaded
UI->>S: dispatch(getChannelsCorruptedThunk)
S->>API: getChannelsCorrupted
API-->>S: corruptedChannels
S-->>UI: corrupted loaded
UI->>S: dispatch(getConfigurationThunk)
S->>API: getConfiguration
API-->>S: configuration
S-->>UI: configuration loaded
Note over R,S: Router also adds getChannelsCorruptedThunk<br/>after getChannelsThunk during login bootstrap
sequenceDiagram
autonumber
participant P as InfoPage
participant S as Store/Redux
participant API as Node API
participant V as View
P->>S: dispatch(getChannelsCorruptedThunk) (on load)
S->>API: getChannelsCorrupted
API-->>S: corruptedChannels
S-->>P: state.channels.corrupted updated
P->>V: Render table rows
V-->V: If corrupted>0 -> show "Faulty RPC | {provider}"
V-->V: Render connectivity status & sync progress with new tooltips
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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. ✨ Finishing touches
🧪 Generate unit tests
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
package.json (1)
3-3: Version mismatch vs PR title and release pipeline.package.json still has 3.0.0 while the PR is “3.0.1”. The release workflow tags from package.json before bumping, so this would tag v3.0.0 again. Bump to 3.0.1 here.
Apply:
- "version": "3.0.0", + "version": "3.0.1",src/components/ConnectNode/modal.tsx (1)
294-329: Use formattedApiEndpoint consistently across all thunksMixed use of
apiEndpoint(raw) vsformattedApiEndpointcan cause subtle failures right after login (e.g., missing protocol or trailing slash). Align all calls to the sanitized value computed earlier inuseNode.Apply this diff:
dispatch( nodeActionsAsync.getChannelsCorruptedThunk({ - apiEndpoint, + apiEndpoint: formattedApiEndpoint, apiToken: apiToken ? apiToken : '', }), ); dispatch( nodeActionsAsync.getConfigurationThunk({ apiToken, - apiEndpoint, + apiEndpoint: formattedApiEndpoint, }), ); dispatch( nodeActionsAsync.getPrometheusMetricsThunk({ - apiEndpoint, + apiEndpoint: formattedApiEndpoint, apiToken: apiToken ? apiToken : '', }), ); dispatch( nodeActionsAsync.getTicketPriceThunk({ apiToken, - apiEndpoint, + apiEndpoint: formattedApiEndpoint, }), ); dispatch( nodeActionsAsync.getMinimumNetworkProbabilityThunk({ - apiEndpoint, + apiEndpoint: formattedApiEndpoint, apiToken: apiToken ? apiToken : '', }), ); dispatch( nodeActionsAsync.getSessionsThunk({ - apiEndpoint, + apiEndpoint: formattedApiEndpoint, apiToken: apiToken ? apiToken : '', }), );src/store/slices/node/actionsAsync.ts (4)
981-1066: Clear incoming link maps to avoid stale entriesOutgoing link map is reset, but incoming maps aren’t. Removed incoming channels will leave stale keys in:
- state.links.nodeAddressToIncomingChannel
- state.links.incomingChannelToNodeAddress
Add resets before rebuilding incoming maps.
Apply this diff:
// Clean store to make sure that removed channels do not stay here state.channels.parsed.outgoing = {}; state.links.nodeAddressToOutgoingChannel = {}; @@ state.channels.parsed.incoming = {}; + // Reset incoming link maps to prevent stale mappings + state.links.nodeAddressToIncomingChannel = {}; + state.links.incomingChannelToNodeAddress = {};Optional: gate the console.log to dev only.
- console.log('getChannels', channels); + if (process.env.NODE_ENV !== 'production') console.log('getChannels', channels);
909-915: isNodeReady: isFetching should be set to false on successCurrently stays true after fulfillment, causing perpetual “loading”.
builder.addCase(isNodeReadyThunk.fulfilled, (state, action) => { // console.log('isNodeReadyThunk', action.payload); if (action.payload) { state.nodeIsReady.data = action.payload; } - state.nodeIsReady.isFetching = true; + state.nodeIsReady.isFetching = false; });
1202-1209: Fix findIndex usage to avoid duplicates and -1 writesfindIndex returns -1 when not found and 0 for the first element; current truthiness check is wrong.
- const tokenExists = state.tokens.data?.findIndex((token) => token.id === action.payload?.id); - - if (tokenExists) { - state.tokens.data[tokenExists] = action.payload; - } else { - state.tokens.data.push(action.payload); - } + const idx = state.tokens.data.findIndex((token) => token.id === action.payload?.id); + if (idx !== -1) { + state.tokens.data[idx] = action.payload; + } else { + state.tokens.data.push(action.payload); + }
1268-1285: Fix findIndex check in pings reducerSame truthiness bug; can overwrite wrong slot or duplicate.
- const pingExists = state.pings.findIndex((ping) => ping.peerId === action.payload?.peerId); + const idx = state.pings.findIndex((ping) => ping.peerId === action.payload?.peerId); if (!action.payload.peerId) return; - if (pingExists) { - state.pings[pingExists] = { + if (idx !== -1) { + state.pings[idx] = { latency: action.payload.latency, peerId: action.payload.peerId, }; } else { state.pings.push({ latency: action.payload.latency, peerId: action.payload.peerId, }); }
🧹 Nitpick comments (13)
Dockerfile (1)
8-10: Nit: drop redundant libc6 install on Debian slim.libc6 is already present on Debian-based images. Keep jq only to reduce layer size.
-RUN apt-get update && \ - apt-get install -y libc6 jq && \ +RUN apt-get update && \ + apt-get install -y jq && \ rm -rf /var/lib/apt/lists/*src/pages/node/sessions.tsx (1)
154-155: Backward-compat for MTU rename.Support both new hoprMtu and older mtu to avoid blank values on older nodes.
- mtu: session.hoprMtu, + mtu: session.hoprMtu ?? (session as any).mtu,src/store/slices/node/initialState.ts (1)
111-114: Use primitive string[] instead of boxed String[].Type with lowercase string for correctness and consistency.
- corrupted: { - data: String[]; - isFetching: boolean; - }; + corrupted: { + data: string[]; + isFetching: boolean; + };src/pages/node/configuration.tsx (4)
5-5: Export helper is OK, but consider Blob-based download for robustnessData-URI +
encodeURIcan misbehave with “#” and large payloads. Optional: move to Blob +URL.createObjectURLinexportToFilefor safer downloads.Outside this file (utils/helpers.ts), consider:
export const exportToFile = (data: string, filename: string, type: string) => { const blob = new Blob([data], { type: `${type};charset=utf-8` }); const url = URL.createObjectURL(blob); const a = document.createElement('a'); a.href = url; a.download = filename; document.body.appendChild(a); a.click(); URL.revokeObjectURL(url); a.remove(); };
58-58: Filename may render as “strategies-undefined.yaml”
myNodeAddresscan be empty before addresses load. Fallback to a stable filename.Apply this diff in handleExport (see lines 170-175):
- if (strategiesString) { - exportToFile(strategiesString, `strategies-${myNodeAddress}.yaml`, 'text/yaml'); - } + if (strategiesString) { + const filename = `strategies-${myNodeAddress || 'node'}.yaml`; + exportToFile(strategiesString, filename, 'text/yaml'); + }
110-144: String-based YAML post-processing is brittle; constrain replacements or transform before dumpThe current global
replacecan hit wrong matches or only the first occurrence. Also, quoting is content-dependent and may miss cases or double-handle future changes.
- Constrain replacements with anchored, key-scoped regex (multiline) to avoid accidental matches.
- Prefer transforming
strategyTMPbeforeyaml.dump(e.g., wrap only the needed values or reshapestrategiesto tag form) instead of mutating the dumped string.If you want a minimal guard now, anchor to list item starts:
- strategiesSet.forEach((strategyName) => { - strategiesString = strategiesString.replace(`- ${strategyName}:`, `- !${strategyName}`); - }); + strategiesSet.forEach((strategyName) => { + const re = new RegExp(`^\\s*-\\s+${strategyName}:`, 'm'); + strategiesString = strategiesString.replace(re, `- !${strategyName}`); + });And for quoting, target the specific key line:
- strategiesString = strategiesString.replace(`${key}: ${strategyValue}`, `${key}: "${strategyValue}"`); + const reKey = new RegExp(`(^\\s*${key}:\\s*)${strategyValue.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}(\\s*$)`, 'm'); + strategiesString = strategiesString.replace(reKey, `$1"${strategyValue}"$2`);
170-175: Add filename fallback and keep type as text/yamlPrevents “undefined” filenames when address isn’t ready.
Apply this diff:
- if (strategiesString) { - exportToFile(strategiesString, `strategies-${myNodeAddress}.yaml`, 'text/yaml'); - } + if (strategiesString) { + const filename = `strategies-${myNodeAddress || 'node'}.yaml`; + exportToFile(strategiesString, filename, 'text/yaml'); + }src/pages/node/info/index.tsx (4)
37-38: Remove unused styled TD
TDis declared but never used.Apply this diff:
-const TD = styled.td``;
249-269: Connectivity tooltip/status text vs stylesTooltip mentions “Yellow/Green” but
ColorStatusonly styles Green/Orange/Red. Add styles (and possibly “Unknown”) or adjust the tooltip to match.Option A (styles in components/InfoBar/details.tsx):
&.status-Green { color: #218520; font-weight: 700; } &.status-Orange { color: #ff8f00; font-weight: 700; } &.status-Red { color: #ff0000; font-weight: 700; } + &.status-Yellow { color: #e6c200; font-weight: 700; } + &.status-Unknown { color: #6b7280; font-weight: 700; }Option B: Remove “Yellow” from the tooltip if it’s not an actual status.
285-292: Nit: tooltip wording“The sync process indexer data source” reads oddly.
Apply this diff:
- title="The sync process indexer data source" + title="The indexer data source used for sync"
296-307: Nit: tooltip wording + labelSmall grammar/readability fixes.
Apply this diff:
- title="The RPC provider address your node uses sync" + title="The RPC provider your node uses to sync" ... - <span>Provider address</span> + <span>RPC provider</span>src/store/slices/node/actionsAsync.ts (2)
1072-1074: Remove unused parameter (nit)action is unused in rejected handler.
- builder.addCase(getChannelsThunk.rejected, (state, action) => { + builder.addCase(getChannelsThunk.rejected, (state) => {
297-298: Strip noisy console.log in production (nit)getConfigurationThunk logs every call. Consider gating or removing.
- console.log('getConfigurationThunk', configuration); + if (process.env.NODE_ENV !== 'production') { + console.log('getConfigurationThunk', configuration); + }
📜 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 (12)
.github/workflows/build.yaml(1 hunks).github/workflows/release.yaml(1 hunks).yarnrc(1 hunks)Dockerfile(2 hunks)package.json(1 hunks)src/components/ConnectNode/modal.tsx(1 hunks)src/pages/node/configuration.tsx(6 hunks)src/pages/node/info/index.tsx(5 hunks)src/pages/node/sessions.tsx(1 hunks)src/router.tsx(1 hunks)src/store/slices/node/actionsAsync.ts(7 hunks)src/store/slices/node/initialState.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T11:16:38.897Z
Learnt from: mjadach-iv
PR: hoprnet/admin-ui#693
File: src/pages/node/configuration.tsx:0-0
Timestamp: 2025-06-09T11:16:38.897Z
Learning: In src/pages/node/configuration.tsx, the delete operator is intentionally used on strategyTMP.hopr.strategy['parsedStrategies'] before YAML serialization because setting to undefined would change the output format, making the delete operator the correct choice despite performance warnings.
Applied to files:
src/pages/node/configuration.tsx
🧬 Code graph analysis (5)
src/components/ConnectNode/modal.tsx (1)
src/store/slices/node/index.ts (1)
nodeActionsAsync(140-140)
src/router.tsx (1)
src/store/slices/node/index.ts (1)
nodeActionsAsync(140-140)
src/pages/node/configuration.tsx (2)
src/utils/helpers.ts (1)
exportToFile(16-24)src/future-hopr-lib-components/Button/IconButton.tsx (1)
IconButton(55-83)
src/store/slices/node/actionsAsync.ts (1)
src/store/index.ts (1)
RootState(20-20)
src/pages/node/info/index.tsx (3)
src/store/index.ts (1)
useAppSelector(26-26)src/store/slices/node/index.ts (1)
nodeActionsAsync(140-140)src/components/InfoBar/details.tsx (1)
ColorStatus(100-113)
🔇 Additional comments (16)
package.json (1)
9-9: SDK prerelease bump: verify registry availability and lockfile sync.Confirm this prerelease exists in the registry used by CI and Release, and ensure yarn.lock is updated accordingly to avoid resolution drift.
.github/workflows/build.yaml (1)
22-22: Node 24 in matrix: sanity-check toolchain compatibility.Vite 4.x, esbuild, and any native deps may lag Node 24 shortly after release. Run CI across both versions (as you do), but verify green builds on 24.x or temporarily gate to 22.x if issues appear.
Dockerfile (1)
1-1: Aligning build stages to Node 22 looks good.Also applies to: 20-20
src/store/slices/node/initialState.ts (1)
316-319: Initialization looks correct.Defaulting corrupted to an empty array and isFetching false is appropriate.
src/router.tsx (1)
314-319: Good integration of getChannelsCorruptedThunk in login flow.Placement after getChannelsThunk keeps sequencing consistent.
src/pages/node/configuration.tsx (2)
14-21: LGTM: UI imports for export actionThe IconButton and GetAppIcon imports fit the intended UX addition.
279-292: Export UI addition looks goodButton placement, tooltip, and action wiring are clear.
src/pages/node/info/index.tsx (4)
67-68: LGTM: Store-derived corrupted-channels flagSimple, fast check that integrates with the new slice shape.
104-109: Good call: fetch corrupted-channels alongside channelsEnsure the thunk degrades gracefully on nodes < 3.0.0 (e.g., 404/unsupported endpoint should not surface as an error toast).
Can you confirm
getChannelsCorruptedThunkswallows 404s/feature-gates internally so older nodes don’t regress?
303-308: LGTM: clear RPC fault indicationConditionally surfacing “Faulty RPC” based on corrupted channels is a useful heads-up.
338-353: LGTM: explicit network rowsSeparating Hopr vs blockchain network improves clarity.
src/store/slices/node/actionsAsync.ts (5)
44-45: New SDK type import looks goodImporting GetChannelsCorruptedResponseType aligns with the new API surface.
64-65: Exposing getChannelsCorrupted from API is correctMatches the new thunk usage below.
261-287: Solid thunk: concurrency guard and error handling are consistentThe condition guard mirrors the pattern used elsewhere, and payload typing is correct.
1075-1086: Corrupted channels reducer cases are correctState scoping, endpoint guard, and defaulting to [] look good.
1429-1430: Exporting getChannelsCorruptedThunk is correctKeeps public actions in sync.
| - name: Setup Node.js | ||
| uses: hoprnet/hopr-workflows/actions/setup-node-js@master | ||
| with: | ||
| node-version: ${{ vars.NODE_VERSION }} | ||
| node-version: 22.x |
There was a problem hiding this comment.
Release job likely missing private registry setup.
Since .yarnrc disables the registry, yarn build here may not resolve @hoprnet/* packages. Use the same private-registry action as CI.
- - name: Setup Node.js
- uses: hoprnet/hopr-workflows/actions/setup-node-js@master
+ - name: Setup Node.js
+ uses: hoprnet/hopr-workflows/actions/setup-node-js@ausias/private-registry
with:
node-version: 22.x📝 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.
| - name: Setup Node.js | |
| uses: hoprnet/hopr-workflows/actions/setup-node-js@master | |
| with: | |
| node-version: ${{ vars.NODE_VERSION }} | |
| node-version: 22.x | |
| - name: Setup Node.js | |
| uses: hoprnet/hopr-workflows/actions/setup-node-js@ausias/private-registry | |
| with: | |
| node-version: 22.x |
🤖 Prompt for AI Agents
.github/workflows/release.yaml around lines 32 to 35: the release job currently
sets up Node.js but does not configure the private package registry, so yarn
will fail resolving @hoprnet/* packages due to .yarnrc blocking the public
registry; add the same private-registry action step used in CI (the
hoprnet/hopr-workflows private-registry action) before running yarn (ideally
before or immediately after Setup Node.js) and pass the same inputs/secrets used
in CI (e.g., registry token/credentials) so the runner can authenticate to the
private registry.
| @@ -1 +1 @@ | |||
| "@hoprnet:registry" "https://europe-west3-npm.pkg.dev/hoprassociation/npm/" | |||
| //"@hoprnet:registry" "https://europe-west3-npm.pkg.dev/hoprassociation/npm/" | |||
There was a problem hiding this comment.
Commenting out the private registry can break Release installs.
Build workflow uses a custom action that likely injects registry config, but release.yaml uses a different action; yarn build there may fail resolving @hoprnet/* prereleases.
Two ways to fix:
Option A — re-enable registry here:
-//"@hoprnet:registry" "https://europe-west3-npm.pkg.dev/hoprassociation/npm/"
+"@hoprnet:registry" "https://europe-west3-npm.pkg.dev/hoprassociation/npm/"Option B — keep this commented and switch Release to the private-registry setup action (preferred for env consistency). See suggested diff in release.yaml comment.
📝 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.
| //"@hoprnet:registry" "https://europe-west3-npm.pkg.dev/hoprassociation/npm/" | |
| "@hoprnet:registry" "https://europe-west3-npm.pkg.dev/hoprassociation/npm/" |
🤖 Prompt for AI Agents
In .yarnrc around lines 1 to 1 the private registry entry for the @hoprnet scope
is commented out which can break Release workflow installs; either uncomment and
restore the "@hoprnet:registry" line with the correct URL so CI and releases can
resolve @hoprnet/* packages, or keep it commented but update release.yaml to use
the same private-registry setup action used by the build workflow (preferred) so
the release job injects the registry config and environment consistently.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores