Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.1.12 #648

Merged
merged 21 commits into from
Dec 3, 2024
Merged

2.1.12 #648

merged 21 commits into from
Dec 3, 2024

Conversation

mjadach-iv
Copy link
Contributor

@mjadach-iv mjadach-iv commented Dec 2, 2024

Changelog:

  • New way of displaying node data in DataTable (peer address and peer id in the same lines in subpages, jazzicons, copy buttons, gnosiscan button)
  • Generate login modal icons based on both apiEndpoint and nodeAddress (apiEndpoint before login, and nodeAddress after)
  • Display the node icon in the navbar based on localStorage data
  • Show whole config in the Configuration page
  • Nicer formatting of hopr values in the strategies
  • Added 'Open swagger UI' and 'Open Scalar UI' to the info page
  • Added sorting of peers, alias and channel pages

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced ConnectNode component to manage and display user-specific data, including jazz icons.
    • Introduced PeersInfo component to display peer information across various pages.
    • Added new functionality for displaying peer aliases in ping operations and channel modals.
  • Improvements

    • Updated table structures in AliasesPage, ChannelsPage, and PeersPage to include new columns for node information.
    • Improved user feedback during login and ping operations by incorporating relevant peer aliases.
    • Enhanced SettingsPage with new utility functions for ticket calculations and strategy updates.
    • Updated OpenChannelModal and PingModal to better handle and display peer-related data.
  • Bug Fixes

    • Refined logic for handling jazz icons and node addresses to ensure accurate display and functionality.
  • Documentation

    • Updated the documentation to reflect changes in components and their props, enhancing clarity for developers.

Copy link

coderabbitai bot commented Dec 2, 2024

📝 Walkthrough

Walkthrough

The pull request introduces significant updates across multiple components to enhance the management and display of user-specific data, particularly focusing on a new property called jazzIcon. Changes include updates to state management, new utility functions, and modifications to component rendering logic. The ConnectNode component and its modal have been adjusted to handle the jazzIcon, while other components like OpenChannelModal, PingModal, and various pages have been updated to integrate this new functionality. New components and utility functions have also been added to improve the overall user experience.

Changes

File Path Change Summary
src/components/ConnectNode/index.tsx Updated state management for local names; added jazzIconFromLocalStorage; modified display logic and useEffect hook.
src/components/ConnectNode/modal.tsx Added jazzIcon to ParsedNode type; updated state and logic to handle node selection and jazzIcon management.
src/components/Modal/node/OpenChannelModal.tsx Introduced selectors for aliases and peerIdToAliasLink; added getAliasByPeerId function.
src/components/Modal/node/PingModal.tsx Added alias handling for ping operations; updated success/error messages to include aliases.
src/future-hopr-lib-components/PeerInfo/index.tsx Introduced PeersInfo component to display peer information, including alias and clipboard functionality.
src/future-hopr-lib-components/Select/index.tsx Updated Props interface to include jazzIcon; modified icon handling logic.
src/future-hopr-lib-components/Table/table-pro.tsx Added sorting capabilities with new state variables and comparator functions.
src/pages/node/aliases.tsx Integrated PeersInfo into the aliases table; updated header configuration.
src/pages/node/channelsIncoming.tsx Enhanced display of peer information with PeersInfo; updated table structure and default ordering.
src/pages/node/channelsOutgoing.tsx Similar updates as channelsIncoming.tsx, integrating PeersInfo and modifying the table structure.
src/pages/node/configuration.tsx Added utility functions for ticket calculations and strategy updates; refactored useEffect for clarity.
src/pages/node/info/index.tsx Added IconButton components for external UIs; modified block number calculation logic.
src/pages/node/peers.tsx Updated to include PeersInfo and modified parsedTableData structure for better peer information representation.
src/router.tsx Enhanced routing logic, added console logs, and improved error handling.
src/store/slices/auth/index.ts Updated useNodeData to handle jazzIcon; added addNodeJazzIcon reducer for managing jazz icons.
src/store/slices/auth/initialState.ts Updated types to include jazzIcon in ADMIN_UI_NODE_LIST and InitialState.
src/store/slices/node/actionsAsync.ts Integrated authActions for handling node jazz icons in asynchronous actions.
src/utils/functions.ts Simplified generateBase64Jazz function by removing intermediate variable; updated parameter name for clarity.

Possibly related PRs

  • Overall improvements in node admin for HOPRd 2.1.5 #640: The changes in this PR involve modifications to the ConnectNode component, including state management and the introduction of a new utility function, generateBase64Jazz, which is also referenced in the main PR. Both PRs enhance the handling of jazz icons within the node components.
  • 2.1.11 #642: This PR includes updates to the ConnectNode component, specifically the addition of jazz icons based on the node API endpoint, which aligns with the changes made in the main PR regarding the management and display of jazz icons.

Suggested reviewers

  • Jaguaras
  • esterlus

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (18)
src/router.tsx (1)

233-233: Consider removing or replacing console.log with a proper logging mechanism

The console.log('Node Admin login from url', formattedApiEndpoint); statement may clutter the console and potentially expose sensitive information. If logging is necessary for debugging or monitoring, consider using a dedicated logging library with appropriate log levels (e.g., debug, info, warn, error).

src/future-hopr-lib-components/Table/table-pro.tsx (3)

163-163: Remove debugging console.log statement

The console.log(a, b, orderBy); statement in the descendingComparator function appears to be a leftover from debugging and should be removed to prevent unnecessary console output and potential performance implications.


158-182: Handle potential undefined or null values in comparator functions

The descendingComparator function does not account for cases where a[orderBy] or b[orderBy] might be undefined or null. This could lead to runtime errors when sorting. Consider adding checks to handle these cases safely.

Apply this diff to handle undefined or null values:

 function descendingComparator<T>(
   a: { [key in string]: number | string },
   b: { [key in string]: number | string },
   orderBy: string,
 ) {
+  const aValue = a[orderBy] ?? '';
+  const bValue = b[orderBy] ?? '';

-  if (isString(b[orderBy]) && isString(a[orderBy])) {
+  if (isString(bValue) && isString(aValue)) {
     if (bValue.toLowerCase() < aValue.toLowerCase()) {
       return -1;
     }
     if (bValue.toLowerCase() > aValue.toLowerCase()) {
       return 1;
     }
   }

   if (bValue < aValue) {
     return -1;
   }
   if (bValue > aValue) {
     return 1;
   }
   return 0;
 }

260-260: Avoid using @ts-ignore; find a type-safe solution

Using @ts-ignore suppresses TypeScript errors but may hide potential issues. Instead, consider defining a proper type for your data that includes possible JSX elements or adjust your data structures to ensure type safety without suppressing errors.

src/store/slices/auth/initialState.ts (1)

21-21: Ensure consistent optionality of jazzIcon property

In the InitialState type, jazzIcon is defined as required (jazzIcon: string | null;) in loginData, but as optional (jazzIcon?: string | null;) in nodes. For consistency and to prevent potential undefined values, consider making jazzIcon consistently required or optional in both places.

Apply this diff for consistency:

 // In the nodes array, make jazzIcon required
-  jazzIcon?: string | null;
+  jazzIcon: string | null;

Also applies to: 27-27

src/future-hopr-lib-components/Select/index.tsx (1)

60-60: Remove debugging console.log statement

The console.log('props.values', props.values); statement is likely intended for debugging and should be removed to prevent unnecessary console output in production.

src/store/slices/auth/index.ts (1)

35-41: Consider extracting the duplicate node lookup logic

The code for finding an existing node by apiEndpoint is duplicated. Consider extracting this into a helper function for better maintainability.

+ const findNodeByApiEndpoint = (nodes: any[], apiEndpoint: string) => 
+   nodes.findIndex((item) => item.apiEndpoint === apiEndpoint);

  // Then use it like:
- const existingItem = state.nodes.findIndex((item) => item.apiEndpoint === action.payload.apiEndpoint);
+ const existingItem = findNodeByApiEndpoint(state.nodes, action.payload.apiEndpoint);
src/utils/functions.ts (1)

Line range hint 155-164: Improve error handling and input validation

The function makes assumptions about the input format and could benefit from better error handling.

  1. Add input validation
  2. Use more specific error handling
  3. Add JSDoc documentation
+ /**
+  * Generates a base64 encoded jazz icon SVG from a hex string
+  * @param input - Hex string (must start with '0x')
+  * @returns Base64 encoded SVG string or null if generation fails
+  */
 export function generateBase64Jazz(input: string) {
   try {
+    if (!input?.startsWith('0x')) {
+      throw new Error('Input must be a hex string starting with 0x');
+    }
     const jazzSvg = jazzicon(16, parseInt(input.slice(2, 10), 16));
     const html = jazzSvg.children[0].outerHTML.replace('<svg', '<svg xmlns="http://www.w3.org/2000/svg"');
     const b64 = 'data:image/svg+xml;base64,' + btoa(html);
     return b64;
   } catch (e) {
-    console.log('Did not manage to create a jazzicon/', e);
+    console.error('Failed to generate jazz icon:', e instanceof Error ? e.message : 'Unknown error');
     return null;
   }
 }
src/components/Modal/node/PingModal.tsx (2)

31-34: Consider memoizing getAliasByPeerId and simplifying conditions

The function could benefit from memoization to prevent unnecessary recalculations, and the conditions could be simplified.

+ import { useMemo } from 'react';

- const getAliasByPeerId = (peerId: string): string => {
-   if (aliases && peerId && peerIdToAliasLink[peerId]) return `${peerIdToAliasLink[peerId]} (${peerId})`;
-   return peerId;
- };
+ const getAliasByPeerId = useMemo(() => (peerId: string): string => {
+   const alias = peerIdToAliasLink[peerId];
+   return alias ? `${alias} (${peerId})` : peerId;
+ }, [peerIdToAliasLink]);

67-67: Consider extracting message formatting logic

The success and error message formatting is duplicated. Consider extracting this into a helper function.

+ const formatPingMessage = (peerId: string, status: 'success' | 'error', details?: string) => {
+   const base = `Ping of ${getAliasByPeerId(peerId)} ${status === 'success' ? 'succeeded' : 'failed'}`;
+   return status === 'success' ? `${base} with latency of ${details}ms` : details ? `${base}.\n${details}` : base;
+ };

- const msg = `Ping of ${getAliasByPeerId(peerId)} succeeded with latency of ${resp.latency}ms`;
+ const msg = formatPingMessage(peerId, 'success', resp.latency);

- let errMsg = `Ping of ${getAliasByPeerId(peerId)} failed`;
+ let errMsg = formatPingMessage(peerId, 'error', e.hoprdErrorPayload?.status || e.hoprdErrorPayload?.error);

Also applies to: 86-86

src/pages/node/peers.tsx (1)

120-124: Consider moving security check to a utility function

The protocol/hostname check for copy-paste functionality would be better placed in a utility function for reusability and maintainability.

+// utils/security.ts
+export const isCopyPasteAllowed = () => 
+  window.location.protocol === 'https:' ||
+  window.location.hostname === 'localhost' ||
+  window.location.hostname === '127.0.0.1';

-  const noCopyPaste = !(
-    window.location.protocol === 'https:' ||
-    window.location.hostname === 'localhost' ||
-    window.location.hostname === '127.0.0.1'
-  );
+  const noCopyPaste = !isCopyPasteAllowed();
src/components/ConnectNode/index.tsx (2)

110-115: Move string truncation logic to a utility function

The local name truncation logic would be better placed in a utility function for reusability and testing.

+// utils/string.ts
+export const truncateString = (str: string, maxLength: number = 17) => {
+  if (!str || str.length <= maxLength) return str;
+  const start = 5;
+  const end = 11;
+  return `${str.substring(0, start)}…${str.substring(str.length - end)}`;
+};

-  const localNameToDisplay =
-    localNameFromLocalStorage && localNameFromLocalStorage.length > 17
-      ? `${localNameFromLocalStorage?.substring(0, 5)}…${localNameFromLocalStorage?.substring(
-          localNameFromLocalStorage.length - 11,
-          localNameFromLocalStorage.length,
-        )}`
-      : localNameFromLocalStorage;
+  const localNameToDisplay = truncateString(localNameFromLocalStorage);

137-145: Optimize useEffect dependencies

The useEffect has multiple dependencies that could trigger unnecessary rerenders. Consider splitting the effect or memoizing some values.

+  const iconSource = useMemo(() => 
+    nodeAddress || jazzIconFromLocalStorage || (apiEndpoint ? toHexMD5(apiEndpoint) : null),
+    [nodeAddress, jazzIconFromLocalStorage, apiEndpoint]
+  );
+
   useEffect(() => {
     if (!connected) set_nodeAddressIcon(null);
-    if (!apiEndpoint) return;
-    const md5 = toHexMD5(apiEndpoint);
-    const b64 = generateBase64Jazz(
-      nodeAddress ? nodeAddress : jazzIconFromLocalStorage ? jazzIconFromLocalStorage : md5,
-    );
-    if (connected && b64) set_nodeAddressIcon(b64);
-  }, [connected, apiEndpoint, nodeAddress, jazzIconFromLocalStorage]);
+    if (!iconSource) return;
+    const b64 = generateBase64Jazz(iconSource);
+    if (connected && b64) set_nodeAddressIcon(b64);
+  }, [connected, iconSource]);
src/pages/node/channelsOutgoing.tsx (2)

152-167: Consider consolidating search fields

The table has multiple hidden columns (peerAddress, peerId) marked for search. Consider consolidating these into a single searchable field for better user experience.


199-214: Extract peer information mapping to a separate function

The peer information mapping logic could be extracted to improve readability and reusability.

+const mapPeerInformation = (peerAddress: string, peerId: string) => ({
+  node: <PeersInfo peerId={peerId} nodeAddress={peerAddress} />,
+  peerAddress: getAliasByPeerAddress(peerAddress),
+  peerId
+});

 const peerAddress = channelsOutgoingObject[id].peerAddress;
 const peerId = getPeerIdFromPeerAddress(peerAddress as string);
+const peerInfo = mapPeerInformation(peerAddress, peerId);

 return {
   id: index.toString(),
   key: id,
   number: parseInt(id),
-  node: (
-    <PeersInfo
-      peerId={peerId}
-      nodeAddress={peerAddress}
-    />
-  ),
-  peerAddress: getAliasByPeerAddress(peerAddress as string),
-  peerId: peerId,
+  ...peerInfo,
src/pages/node/channelsIncoming.tsx (2)

Line range hint 214-218: Consider using BigInt.toString() for consistent formatting

The ticket calculations involve multiple BigInt operations and string conversions. Consider using a consistent formatting approach.

-const totalTicketsPerChannel = `${formatEther(
-  BigInt(tickets?.redeemed[id]?.value || '0') + BigInt(tickets?.unredeemed[id]?.value || '0'),
-)}`;
+const getTotalTickets = (redeemed: string = '0', unredeemed: string = '0') => {
+  const total = BigInt(redeemed) + BigInt(unredeemed);
+  return formatEther(total);
+};
+const totalTicketsPerChannel = getTotalTickets(
+  tickets?.redeemed[id]?.value,
+  tickets?.unredeemed[id]?.value
+);

106-122: Consider creating a shared configuration for channel tables

Both incoming and outgoing channel tables share similar column configurations and data mapping logic. Consider extracting these to a shared configuration.

Create a new file src/config/channelTables.ts to share common configurations:

export const commonColumns = {
  node: {
    key: 'node',
    name: 'Node',
    maxWidth: '350px',
  },
  peerAddress: {
    key: 'peerAddress',
    name: 'Node Address',
    search: true,
    hidden: true,
  },
  // ... other common columns
};

export const mapPeerInfo = (peerAddress: string, peerId: string, getAlias: (addr: string) => string) => ({
  node: <PeersInfo peerId={peerId} nodeAddress={peerAddress} />,
  peerAddress: getAlias(peerAddress),
  peerId,
});

Also applies to: 222-231

src/pages/node/info/index.tsx (1)

206-207: Remove unnecessary @ts-ignore comments.

The TypeScript errors should be properly handled instead of being ignored.

Consider fixing the type issues:

-//@ts-ignore
+onClick={() => {
+  const win = window.open(url, '_blank');
+  if (win) win.focus();
+}}

Also applies to: 225-226

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4bc9775 and 3ae68be.

⛔ Files ignored due to path filters (1)
  • public/assets/scalar-removebg-preview.png is excluded by !**/*.png
📒 Files selected for processing (18)
  • src/components/ConnectNode/index.tsx (4 hunks)
  • src/components/ConnectNode/modal.tsx (6 hunks)
  • src/components/Modal/node/OpenChannelModal.tsx (1 hunks)
  • src/components/Modal/node/PingModal.tsx (3 hunks)
  • src/future-hopr-lib-components/PeerInfo/index.tsx (1 hunks)
  • src/future-hopr-lib-components/Select/index.tsx (4 hunks)
  • src/future-hopr-lib-components/Table/table-pro.tsx (4 hunks)
  • src/pages/node/aliases.tsx (4 hunks)
  • src/pages/node/channelsIncoming.tsx (6 hunks)
  • src/pages/node/channelsOutgoing.tsx (4 hunks)
  • src/pages/node/configuration.tsx (5 hunks)
  • src/pages/node/info/index.tsx (3 hunks)
  • src/pages/node/peers.tsx (4 hunks)
  • src/router.tsx (1 hunks)
  • src/store/slices/auth/index.ts (3 hunks)
  • src/store/slices/auth/initialState.ts (3 hunks)
  • src/store/slices/node/actionsAsync.ts (2 hunks)
  • src/utils/functions.ts (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/node/configuration.tsx

[error] 107-107: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

🔇 Additional comments (13)
src/components/Modal/node/OpenChannelModal.tsx (1)

31-32: LGTM! Clean implementation of alias support

The new selectors and helper function enhance the modal with proper alias display capabilities while following Redux best practices.

Also applies to: 38-41

src/pages/node/peers.tsx (1)

91-92: Verify accessibility impact of hidden columns

Hidden columns containing peerId and peerAddress might affect screen readers. Consider adding aria-labels to the PeersInfo component to maintain accessibility.

Also applies to: 97-98

src/components/ConnectNode/index.tsx (1)

65-66: LGTM! Good CSS practices

The added CSS properties ensure consistent height and prevent text wrapping, which improves the UI stability.

Also applies to: 71-73

src/pages/node/configuration.tsx (1)

37-45: Potential edge case in updateStrategyString function

The function assumes the exact string format "${key}": "${value} HOPR" exists in the input. This might break if the input format changes.

src/pages/node/aliases.tsx (3)

130-135: LGTM: Improved node information display using PeersInfo component.

The integration of the PeersInfo component enhances the UI by consolidating the peer ID and node address display.


179-212: LGTM: Well-structured table header configuration.

The header configuration is well-organized with appropriate column properties:

  • Proper search flags
  • Reasonable column widths
  • Hidden redundant columns (peerId and peerAddress) as they're now shown in the PeersInfo component

248-248: LGTM: Added user-friendly default sorting.

The addition of orderByDefault="alias" improves user experience by providing a sensible default sort order.

Also applies to: 250-250

src/components/ConnectNode/modal.tsx (3)

33-33: LGTM: Added jazzIcon to ParsedNode type.

The jazzIcon property is correctly added as an optional field to the ParsedNode type.


142-142: LGTM: Improved UX with loading state.

The loginPending state is properly used to show loading state during login attempts.

Also applies to: 443-443


145-145: LGTM: Comprehensive jazzIcon implementation.

The jazzIcon feature is well-implemented across the component:

  • Proper state initialization from loginData
  • Correct inclusion in parsed node data
  • Proper state updates during node selection
  • Correct inclusion in dispatched node data

Also applies to: 162-162, 268-268, 358-358

src/pages/node/info/index.tsx (1)

65-68: LGTM: Improved block number calculation logic.

The calculation of blockNumberIndexedWithHOPRdata now correctly handles the increment condition.

src/store/slices/node/actionsAsync.ts (2)

51-51: LGTM: Clean import addition

The import of authActions is properly added and aligns with the new Jazz icon functionality.


150-157: Verify error handling for the Jazz icon dispatch

The implementation looks good, but let's ensure proper error handling for the Jazz icon functionality.

Let's verify the error handling implementation in the auth slice:

Additionally, let's verify the usage of this new functionality:

✅ Verification successful

Let me check the error handling implementation more thoroughly by looking at the auth slice reducer.


Let me try a different pattern to get the reducer implementation.


Let me fix the regex syntax.


Let me try one last approach to get the reducer implementation.


Error handling for Jazz icon dispatch is properly implemented

The implementation has appropriate error handling:

  • Validates the existence of the node before updating (existingItem !== -1)
  • Validates the Jazz icon address using isAddress check
  • Safely updates local storage with stringified state
  • Handles both address and non-address Jazz icon formats
  • Gracefully returns if node is not found without throwing errors
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check error handling in auth slice for Jazz icon
# Test: Look for error handling patterns in the auth slice
rg -A 5 "addNodeJazzIcon" src/store/slices/auth/

Length of output: 363


Script:

#!/bin/bash
# Description: Check usage of Jazz icon in components
# Test: Look for components using the Jazz icon data
rg "jazzIcon" src/

Length of output: 2580


Script:

#!/bin/bash
# Get the full implementation of addNodeJazzIcon reducer
ast-grep --pattern 'addNodeJazzIcon(state, action: $_) {
  $$$
}'

Length of output: 67


Script:

#!/bin/bash
# Get the full implementation using ripgrep with context
rg -A 15 "addNodeJazzIcon.*{" src/store/slices/auth/

Length of output: 154


Script:

#!/bin/bash
# Get the full implementation using ripgrep with context
rg -A 15 "addNodeJazzIcon.*\{" src/store/slices/auth/

Length of output: 55


Script:

#!/bin/bash
# Get content of the auth slice file
cat src/store/slices/auth/index.ts

Length of output: 4313

Comment on lines +57 to +58
onClick={() => navigator.clipboard.writeText(peerId as string)}
disabled={noCopyPaste}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add checks for undefined peerId and nodeAddress before use

Since peerId and nodeAddress are optional and might be undefined, directly using them in navigator.clipboard.writeText can lead to runtime errors. Ensure these values are defined before attempting to use them.

Apply this diff to add null checks:

- onClick={() => navigator.clipboard.writeText(peerId as string)}
+ onClick={() => { if (peerId) navigator.clipboard.writeText(peerId); }}

- onClick={() => navigator.clipboard.writeText(nodeAddress as string)}
+ onClick={() => { if (nodeAddress) navigator.clipboard.writeText(nodeAddress); }}

Also applies to: 66-67

Comment on lines +88 to +92
if (isAddress(action.payload.jazzIcon)) {
state.nodes[existingItem].jazzIcon = action.payload.jazzIcon;
} else {
state.nodes[existingItem].jazzIcon = action.payload.jazzIcon;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify redundant conditional logic

The if-else condition is redundant as both branches perform the same operation.

- if (isAddress(action.payload.jazzIcon)) {
-   state.nodes[existingItem].jazzIcon = action.payload.jazzIcon;
- } else {
-   state.nodes[existingItem].jazzIcon = action.payload.jazzIcon;
- }
+ state.nodes[existingItem].jazzIcon = action.payload.jazzIcon;

If there was intended to be different behavior based on whether the input is an address, please clarify the requirements.

📝 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
if (isAddress(action.payload.jazzIcon)) {
state.nodes[existingItem].jazzIcon = action.payload.jazzIcon;
} else {
state.nodes[existingItem].jazzIcon = action.payload.jazzIcon;
}
state.nodes[existingItem].jazzIcon = action.payload.jazzIcon;

const parsedTableData = Object.entries(peers?.connected ?? {}).map(([id, peer]) => {
return {
id: id,
number: id,
number: parseInt(id),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add radix parameter to parseInt

Always specify the radix parameter in parseInt to ensure consistent parsing behavior across browsers.

-      number: parseInt(id),
+      number: parseInt(id, 10),
📝 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
number: parseInt(id),
number: parseInt(id, 10),

const b64 = generateBase64Jazz(apiEndpoint);
if (b64) set_nodeAddressIcon(b64);
}, [apiEndpoint]);
console.log(jazzIconFromLocalStorage);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove debug console.log statement

Debug console.log statement should be removed before merging to production.

-    console.log(jazzIconFromLocalStorage);

useEffect(() => {
if (configuration) {
let tmp = JSON.parse(JSON.stringify(configuration));
tmp.hopr['strategy'] && delete tmp.hopr['strategy'];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace delete operator with undefined assignment

Using the delete operator can impact performance. Consider using undefined assignment instead.

-tmp.hopr['strategy'] && delete tmp.hopr['strategy'];
+tmp.hopr['strategy'] && (tmp.hopr['strategy'] = undefined);
📝 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
tmp.hopr['strategy'] && delete tmp.hopr['strategy'];
tmp.hopr['strategy'] && (tmp.hopr['strategy'] = undefined);
🧰 Tools
🪛 Biome (1.9.4)

[error] 107-107: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

Comment on lines +24 to +35
const DECIMALS_MULTIPLIER = BigInt(1e18); // For HOPR token's 18 decimals

interface StrategyConfig {
path: ['AutoFunding' | 'AutoRedeeming', string];
value: string;
}

const calculateTickets = (value: string, ticketPrice: string) => {
const valueBigInt = BigInt(value) * DECIMALS_MULTIPLIER;
const ticketBigInt = BigInt(ticketPrice);
return valueBigInt / ticketBigInt;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding input validation for calculateTickets function

The function performs BigInt operations without validating input strings, which could throw runtime errors if invalid inputs are provided.

 const calculateTickets = (value: string, ticketPrice: string) => {
+  if (!value || !ticketPrice || isNaN(Number(value)) || isNaN(Number(ticketPrice))) {
+    throw new Error('Invalid input: value and ticketPrice must be valid numeric strings');
+  }
   const valueBigInt = BigInt(value) * DECIMALS_MULTIPLIER;
   const ticketBigInt = BigInt(ticketPrice);
   return valueBigInt / ticketBigInt;
 };
📝 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 DECIMALS_MULTIPLIER = BigInt(1e18); // For HOPR token's 18 decimals
interface StrategyConfig {
path: ['AutoFunding' | 'AutoRedeeming', string];
value: string;
}
const calculateTickets = (value: string, ticketPrice: string) => {
const valueBigInt = BigInt(value) * DECIMALS_MULTIPLIER;
const ticketBigInt = BigInt(ticketPrice);
return valueBigInt / ticketBigInt;
};
const DECIMALS_MULTIPLIER = BigInt(1e18); // For HOPR token's 18 decimals
interface StrategyConfig {
path: ['AutoFunding' | 'AutoRedeeming', string];
value: string;
}
const calculateTickets = (value: string, ticketPrice: string) => {
if (!value || !ticketPrice || isNaN(Number(value)) || isNaN(Number(ticketPrice))) {
throw new Error('Invalid input: value and ticketPrice must be valid numeric strings');
}
const valueBigInt = BigInt(value) * DECIMALS_MULTIPLIER;
const ticketBigInt = BigInt(ticketPrice);
return valueBigInt / ticketBigInt;
};

Comment on lines 194 to 231
actions={
<>
<WithdrawModal />
<IconButton
iconComponent={<DataObjectIcon />}
tooltipText={
<span>
OPEN
<br />
Swagger UI
</span>
}
//@ts-ignore
onClick={() => {
window.open(apiEndpoint + '/swagger-ui/index.html#/', '_blank').focus();
}}
/>
<IconButton
iconComponent={
<img
style={{ maxWidth: '20px' }}
src="/assets/scalar-removebg-preview.png"
/>
}
tooltipText={
<span>
OPEN
<br />
Scalar UI
</span>
}
//@ts-ignore
onClick={() => {
window.open(apiEndpoint + '/scalar', '_blank').focus();
}}
/>
</>
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security concern: Potential XSS vulnerability in URL handling.

The direct concatenation of user-controlled apiEndpoint with static paths could lead to XSS if the endpoint contains malicious content.

Consider sanitizing the URL before opening:

-window.open(apiEndpoint + '/swagger-ui/index.html#/', '_blank').focus();
+const swaggerUrl = new URL('/swagger-ui/index.html#/', apiEndpoint).toString();
+window.open(swaggerUrl, '_blank')?.focus();

-window.open(apiEndpoint + '/scalar', '_blank').focus();
+const scalarUrl = new URL('/scalar', apiEndpoint).toString();
+window.open(scalarUrl, '_blank')?.focus();
📝 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
actions={
<>
<WithdrawModal />
<IconButton
iconComponent={<DataObjectIcon />}
tooltipText={
<span>
OPEN
<br />
Swagger UI
</span>
}
//@ts-ignore
onClick={() => {
window.open(apiEndpoint + '/swagger-ui/index.html#/', '_blank').focus();
}}
/>
<IconButton
iconComponent={
<img
style={{ maxWidth: '20px' }}
src="/assets/scalar-removebg-preview.png"
/>
}
tooltipText={
<span>
OPEN
<br />
Scalar UI
</span>
}
//@ts-ignore
onClick={() => {
window.open(apiEndpoint + '/scalar', '_blank').focus();
}}
/>
</>
}
actions={
<>
<WithdrawModal />
<IconButton
iconComponent={<DataObjectIcon />}
tooltipText={
<span>
OPEN
<br />
Swagger UI
</span>
}
//@ts-ignore
onClick={() => {
const swaggerUrl = new URL('/swagger-ui/index.html#/', apiEndpoint).toString();
window.open(swaggerUrl, '_blank')?.focus();
}}
/>
<IconButton
iconComponent={
<img
style={{ maxWidth: '20px' }}
src="/assets/scalar-removebg-preview.png"
/>
}
tooltipText={
<span>
OPEN
<br />
Scalar UI
</span>
}
//@ts-ignore
onClick={() => {
const scalarUrl = new URL('/scalar', apiEndpoint).toString();
window.open(scalarUrl, '_blank')?.focus();
}}
/>
</>
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
src/pages/node/info/index.tsx (1)

214-217: Improve image implementation

  1. Move inline styles to styled components
  2. Add alt text for accessibility
-                <img
-                  style={{ maxWidth: '20px' }}
-                  src="/assets/scalar-removebg-preview.png"
-                />
+                <img
+                  css={{ maxWidth: '20px' }}
+                  src="/assets/scalar-removebg-preview.png"
+                  alt="Scalar UI"
+                />
src/future-hopr-lib-components/Table/table-pro.tsx (3)

158-180: Improve type safety in the comparator function

The comparator implementation is good, but there are opportunities to improve type safety:

  1. The function assumes values are either strings or numbers without proper type checking
  2. The type assertion in string comparison could be unsafe

Consider this safer implementation:

 function descendingComparator<T>(
-  a: { [key in string]: number | string },
-  b: { [key in string]: number | string },
+  a: { [key in string]: number | string | JSX.Element },
+  b: { [key in string]: number | string | JSX.Element },
   orderBy: string,
 ) {
-  if (isString(b[orderBy]) && isString(a[orderBy])) {
+  const aVal = a[orderBy];
+  const bVal = b[orderBy];
+  
+  // Skip comparison for non-comparable types
+  if (React.isValidElement(aVal) || React.isValidElement(bVal)) {
+    return 0;
+  }
+
+  if (typeof bVal === 'string' && typeof aVal === 'string') {
-    if ((b[orderBy] as string).toLowerCase() < (a[orderBy] as string).toLowerCase()) {
+    if (bVal.toLowerCase() < aVal.toLowerCase()) {
       return -1;
     }
-    if ((b[orderBy] as string).toLowerCase() > (a[orderBy] as string).toLowerCase()) {
+    if (bVal.toLowerCase() > aVal.toLowerCase()) {
       return 1;
     }
   }

-  if (b[orderBy] < a[orderBy]) {
+  if (typeof bVal === 'number' && typeof aVal === 'number' && bVal < aVal) {
     return -1;
   }
-  if (b[orderBy] > a[orderBy]) {
+  if (typeof bVal === 'number' && typeof aVal === 'number' && bVal > aVal) {
     return 1;
   }

   return 0;
 }

255-262: Remove @ts-ignore by improving type safety

While the memoization implementation is good, the TypeScript ignore comment could be avoided with proper typing.

Consider this type-safe implementation:

 const visibleRows = React.useMemo(
   () =>
     [...filteredData]
-      //@ts-ignore as we can input JSX into the data, but we will not sort by it
-      .sort(getComparator(order, orderBy))
+      .sort((a, b) => {
+        const aVal = a[orderBy];
+        const bVal = b[orderBy];
+        // Skip sorting for JSX elements
+        if (React.isValidElement(aVal) || React.isValidElement(bVal)) {
+          return 0;
+        }
+        return getComparator(order, orderBy)(a, b);
+      })
       .slice(page * rowsPerPage, rowsPerPage !== -1 ? page * rowsPerPage + rowsPerPage : filteredData.length),
   [rowsPerPage, filteredData, order, orderBy, page, rowsPerPage],
 );

Line range hint 303-319: Add sorting UI elements to table headers

The table implements sorting functionality but lacks UI elements to allow users to sort columns by clicking headers.

Consider adding sort indicators and click handlers to the table headers:

 <TableRow>
   {props.header.map(
     (headElem, idx) =>
       !headElem.hidden && (
         <STableCell
           key={idx}
           className={`TableCell TableCellHeader`}
           width={headElem?.width ?? ''}
+          onClick={() => {
+            const isAsc = orderBy === headElem.key && order === 'asc';
+            setOrder(isAsc ? 'desc' : 'asc');
+            setOrderBy(headElem.key);
+          }}
+          style={{ cursor: 'pointer' }}
         >
           <Tooltip
             title={headElem.tooltipHeader}
             notWide
           >
-            <span>{headElem.name}</span>
+            <Box sx={{ display: 'flex', alignItems: 'center' }}>
+              <span>{headElem.name}</span>
+              {orderBy === headElem.key ? (
+                <Box component="span" sx={{ ml: 1 }}>
+                  {order === 'desc' ? '↓' : '↑'}
+                </Box>
+              ) : null}
+            </Box>
           </Tooltip>
         </STableCell>
       ),
   )}
 </TableRow>
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3ae68be and aa6036e.

📒 Files selected for processing (7)
  • config.ts (1 hunks)
  • package.json (1 hunks)
  • scripts/post-build.js (0 hunks)
  • scripts/prepare-build-hub.js (0 hunks)
  • scripts/prepare-build-node.js (0 hunks)
  • src/future-hopr-lib-components/Table/table-pro.tsx (5 hunks)
  • src/pages/node/info/index.tsx (3 hunks)
💤 Files with no reviewable changes (3)
  • scripts/post-build.js
  • scripts/prepare-build-node.js
  • scripts/prepare-build-hub.js
✅ Files skipped from review due to trivial changes (1)
  • config.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/node/info/index.tsx

[error] 209-209: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 229-229: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (6)
src/pages/node/info/index.tsx (2)

65-68: LGTM: Improved block number calculation logic

The updated logic for blockNumberIndexedWithHOPRdata now includes a proper check against blockNumberFromInfo, preventing unnecessary increments.


197-231: ⚠️ Potential issue

Security and code quality improvements needed

  1. The URL construction is vulnerable to XSS attacks. Use URL constructor for safe URL handling.
  2. Use optional chaining for window.open calls.

Apply these improvements:

-              onClick={() => {
-                const externalUrl = apiEndpoint + '/swagger-ui/index.html#/';
-                const w = window.open(externalUrl, '_blank');
-                w && w.focus();
-              }}
+              onClick={() => {
+                const swaggerUrl = new URL('/swagger-ui/index.html#/', apiEndpoint).toString();
+                window.open(swaggerUrl, '_blank')?.focus();
+              }}

-              onClick={() => {
-                const externalUrl = apiEndpoint + '/scalar';
-                const w = window.open(externalUrl, '_blank');
-                w && w.focus();
-              }}
+              onClick={() => {
+                const scalarUrl = new URL('/scalar', apiEndpoint).toString();
+                window.open(scalarUrl, '_blank')?.focus();
+              }}
🧰 Tools
🪛 Biome (1.9.4)

[error] 209-209: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 229-229: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

package.json (2)

52-52: Verify the impact of simplified build script.

The build script has been significantly simplified by removing the preparation and post-build steps. Please ensure that:

  1. All necessary build artifacts are still generated correctly
  2. The deployment process is not affected by this change

Run the following script to check for removed build scripts and their usage:

✅ Verification successful

Based on the results, I can now generate the final response since we have enough information to make a determination:

Build script simplification is safe and aligned with modern Vite practices

The simplified build script "build": "tsc && vite build" is appropriate because:

  • The vite.config.ts shows a proper configuration with all necessary build settings including output directory and plugins
  • No custom build scripts directory exists in the project
  • The build process follows Vite's standard approach: TypeScript compilation followed by Vite build
  • The configuration includes all necessary plugins for React, TypeScript checking, SVG handling, and Node.js polyfills
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for references to removed build scripts and verify their importance

# Check for references to removed script files
echo "Checking for references to removed build scripts..."
rg -l "prepare-build-node.js|prepare-build-hub.js|post-build.js"

# Check for potential build-related files that might be affected
echo "Checking for build-related configuration files..."
fd -e config.js -e config.ts

Length of output: 334


Script:

#!/bin/bash
# Let's check the vite.config.ts and package.json history to understand the build changes

# Check vite.config.ts content
echo "=== Vite Config Content ==="
cat vite.config.ts

# Check git history for build-related changes in package.json
echo -e "\n=== Recent Build Script Changes ==="
git log -p --no-merges -G "\"build\":" package.json | head -n 50

# Check for any build-related files in scripts directory
echo -e "\n=== Build-related Files in Scripts Directory ==="
fd -t f . scripts/

Length of output: 1344


Line range hint 1-100: Verify dependency compatibility with updated versions.

The updates to @hoprnet/hopr-sdk (2.1.9) and ethers (5.7.2) appear to be minor version bumps. The presence of @metamask/jazzicon aligns with the new jazzicon feature mentioned in the PR objectives.

Run the following script to check for potential compatibility issues:

✅ Verification successful

Let's analyze the actual usage patterns of these dependencies to verify compatibility.


Dependencies are compatible with the updated versions

The analysis shows that the codebase uses stable APIs from both @hoprnet/hopr-sdk and ethers:

  • SDK usage is primarily focused on stable utility functions (utils, sdkApiError) and type definitions
  • Ethers usage is limited to basic utilities (utils, standard provider/contract interactions)
  • No direct instantiation of SDK classes or usage of experimental features
  • The imports and usage patterns suggest these are non-breaking version updates

The presence of @metamask/jazzicon aligns with the PR objectives, and its integration doesn't conflict with other dependencies.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for compatibility between dependencies

# Check for peer dependency requirements
echo "Checking package-lock.json or yarn.lock for dependency tree..."
fd -e package-lock.json -e yarn.lock

# Look for imports of updated packages to verify usage
echo "Checking usage of updated packages..."
rg -l "@hoprnet/hopr-sdk|ethers"

Length of output: 1037


Script:

#!/bin/bash
# Check for specific version requirements in the codebase
echo "Checking for specific version imports or requirements..."
rg -A 2 "require.*@hoprnet/hopr-sdk|import.*@hoprnet/hopr-sdk"

echo "Checking ethers usage patterns..."
rg -A 2 "import.*ethers"

# Check for potential breaking changes in usage
echo "Checking SDK method usage..."
rg "new Hopr|HoprSDK"

# Check package.json in the repository for any peer dependency specifications
echo "Checking peer dependencies..."
rg '"peerDependencies"' -A 5 package.json

Length of output: 6634

src/future-hopr-lib-components/Table/table-pro.tsx (2)

131-131: LGTM: Type extension for numeric support

The addition of the number type to the data property is a good enhancement that allows the table to handle numeric columns properly.


197-198: LGTM: Well-structured sorting state management

The sorting state implementation is clean and follows React best practices. The default value fallback chain (props.orderByDefault || props.header[0].key || 'id') provides good flexibility.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between aa6036e and 7fe7f2a.

📒 Files selected for processing (5)
  • src/future-hopr-lib-components/PeerInfo/index.tsx (1 hunks)
  • src/future-hopr-lib-components/Table/table-pro.tsx (7 hunks)
  • src/pages/node/channelsIncoming.tsx (6 hunks)
  • src/pages/node/channelsOutgoing.tsx (4 hunks)
  • src/pages/node/peers.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/pages/node/channelsIncoming.tsx
🔇 Additional comments (1)
src/future-hopr-lib-components/PeerInfo/index.tsx (1)

60-61: ⚠️ Potential issue

Add null checks for 'peerId' and 'nodeAddress' before using 'navigator.clipboard.writeText'

Since peerId and nodeAddress are optional and may be undefined, directly passing them to navigator.clipboard.writeText can lead to runtime errors when they are undefined.

Apply this diff to add null checks:

-              onClick={() => navigator.clipboard.writeText(peerId as string)}
+              onClick={() => { if (peerId) navigator.clipboard.writeText(peerId); }}

-              onClick={() => navigator.clipboard.writeText(nodeAddress as string)}
+              onClick={() => { if (nodeAddress) navigator.clipboard.writeText(nodeAddress); }}

Also applies to: 69-70

src/pages/node/peers.tsx Outdated Show resolved Hide resolved
src/pages/node/channelsOutgoing.tsx Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7fe7f2a and 4397c23.

📒 Files selected for processing (4)
  • src/future-hopr-lib-components/PeerInfo/index.tsx (1 hunks)
  • src/pages/node/channelsIncoming.tsx (7 hunks)
  • src/pages/node/channelsOutgoing.tsx (5 hunks)
  • src/pages/node/peers.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/pages/node/peers.tsx
  • src/pages/node/channelsIncoming.tsx
🔇 Additional comments (2)
src/pages/node/channelsOutgoing.tsx (1)

357-357: Correct the 'orderByDefault' property to match the header key

The orderByDefault is set to "number", but there is no header key named "number". The key for the identifier column is 'id'. This mismatch could cause the default sorting to not work as expected.

Apply this diff to fix the issue:

-            orderByDefault="number"
+            orderByDefault="id"
src/future-hopr-lib-components/PeerInfo/index.tsx (1)

59-60: Add checks for undefined peerId and nodeAddress before use

Since peerId and nodeAddress are optional and might be undefined, directly using them in navigator.clipboard.writeText can lead to runtime errors. Ensure these values are defined before attempting to use them.

Apply this diff to add null checks:

-              onClick={() => navigator.clipboard.writeText(peerId as string)}
+              onClick={() => { if (peerId) navigator.clipboard.writeText(peerId); }}

-              onClick={() => navigator.clipboard.writeText(nodeAddress as string)}
+              onClick={() => { if (nodeAddress) navigator.clipboard.writeText(nodeAddress); }}

Also applies to: 68-69

Comment on lines +195 to +203
const peersWithAliasesSorted = peersWithAliases.sort((a, b) => {
if (getAliasByPeerAddress(b.peerAddress).toLowerCase() > getAliasByPeerAddress(a.peerAddress).toLowerCase()) {
return -1;
}
if (getAliasByPeerAddress(b.peerAddress).toLowerCase() < getAliasByPeerAddress(a.peerAddress).toLowerCase()) {
return 1;
}
return 0;
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the sorting logic in the sort functions

The comparison functions in peersWithAliasesSorted and peersWithoutAliasesSorted reverse the operands, which may result in incorrect sorting order. To sort in ascending order, compare a to b instead of b to a.

Apply this diff to fix the sorting logic:

-peersWithAliasesSorted = peersWithAliases.sort((a, b) => {
-  if (getAliasByPeerAddress(b.peerAddress).toLowerCase() > getAliasByPeerAddress(a.peerAddress).toLowerCase()) {
-    return -1;
-  }
-  if (getAliasByPeerAddress(b.peerAddress).toLowerCase() < getAliasByPeerAddress(a.peerAddress).toLowerCase()) {
-    return 1;
-  }
-  return 0;
-});
+peersWithAliasesSorted = peersWithAliases.sort((a, b) => {
+  if (getAliasByPeerAddress(a.peerAddress).toLowerCase() < getAliasByPeerAddress(b.peerAddress).toLowerCase()) {
+    return -1;
+  }
+  if (getAliasByPeerAddress(a.peerAddress).toLowerCase() > getAliasByPeerAddress(b.peerAddress).toLowerCase()) {
+    return 1;
+  }
+  return 0;
+});

Similarly, apply the same correction to the peersWithoutAliasesSorted function:

-peersWithoutAliasesSorted = peersWithoutAliases.sort((a, b) => {
-  if (b.peerAddress > a.peerAddress) {
-    return -1;
-  }
-  if (b.peerAddress < a.peerAddress) {
-    return 1;
-  }
-  return 0;
-});
+peersWithoutAliasesSorted = peersWithoutAliases.sort((a, b) => {
+  if (a.peerAddress < b.peerAddress) {
+    return -1;
+  }
+  if (a.peerAddress > b.peerAddress) {
+    return 1;
+  }
+  return 0;
+});

Also applies to: 207-215

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
src/future-hopr-lib-components/PeerInfo/index.tsx (1)

14-18: Consider adding prop validation for nodeAddress

The nodeAddress prop is used to generate icons and create external links. Consider adding validation or documentation to specify the expected format.

Add JSDoc comments to document the expected format:

+/**
+ * @param props.nodeAddress - Ethereum address of the node
+ * @param props.peerId - Unique identifier of the peer
+ * @param props.shortenPeerId - Whether to display shortened version of peerId
+ */
interface Props {
  peerId?: string;
  nodeAddress?: string;
  shortenPeerId?: boolean;
}
src/store/slices/node/initialState.ts (1)

147-166: Document the peer information structure and address TODO

The TODO indicates a missing type in the SDK. This should be tracked and implemented in the SDK.

  1. Would you like me to create an issue to track the addition of ConnectedPeerType to the SDK?
  2. Consider adding documentation for the peer information structure:
/**
 * Parsed peer information structure
 * @property connected - Map of connected peers indexed by peerId
 * @property connected[peerId].quality - Connection quality metric (0-1)
 * @property connected[peerId].heartbeats - Connection health monitoring
 * @property connected[peerId].lastSeen - Timestamp of last activity
 * @property connected[peerId].backoff - Backoff period for reconnection attempts
 */
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4397c23 and 69f2cdc.

📒 Files selected for processing (5)
  • src/future-hopr-lib-components/PeerInfo/index.tsx (1 hunks)
  • src/pages/node/aliases.tsx (5 hunks)
  • src/pages/node/peers.tsx (3 hunks)
  • src/store/slices/node/actionsAsync.ts (3 hunks)
  • src/store/slices/node/initialState.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/pages/node/aliases.tsx
  • src/store/slices/node/actionsAsync.ts
  • src/pages/node/peers.tsx
🔇 Additional comments (4)
src/future-hopr-lib-components/PeerInfo/index.tsx (3)

20-27: LGTM!

The styled component implementation is clean and follows flex layout best practices.


64-65: ⚠️ Potential issue

Add checks for undefined peerId and nodeAddress before use

Since peerId and nodeAddress are optional and might be undefined, directly using them in navigator.clipboard.writeText can lead to runtime errors.

Also applies to: 73-74


79-86: ⚠️ Potential issue

Add URL sanitization for external links

Direct concatenation of the nodeAddress into URLs could potentially lead to security issues if the address contains malicious characters.

Consider adding validation:

+const isValidAddress = (address?: string) => {
+  return address?.match(/^0x[a-fA-F0-9]{40}$/);
+};

-          <Link
-            to={`https://gnosisscan.io/address/${nodeAddress}`}
+          <Link
+            to={isValidAddress(nodeAddress) 
+              ? `https://gnosisscan.io/address/${nodeAddress}`
+              : '#'}
+            onClick={(e) => !isValidAddress(nodeAddress) && e.preventDefault()}

Likely invalid or redundant comment.

src/store/slices/node/initialState.ts (1)

312-314: Verify migration from array to object structure

The change from array to object structure for peers.data.connected is a breaking change that might affect existing code.

Let's check for usage of the old array structure:

✅ Verification successful

Change from array to object structure appears safe

Based on the codebase analysis, the change is safe as the existing code already uses object-style access:

  • In actionsAsync.ts, data is stored using object indexing: state.peers.parsed.connected[node.peerId] = node
  • In aliases.tsx, data is accessed as an object through useAppSelector

No array-specific operations (map, filter, etc.) or numeric indexing were found in the codebase, indicating the previous array declaration might have been incorrect and this change actually aligns the type with its actual usage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for array operations on peers.data.connected
rg -l "peers\.data\.connected\.(map|filter|forEach|reduce|some|every|find|findIndex)"

# Search for direct array access
rg -l "peers\.data\.connected\[\d+\]"

Length of output: 127


Script:

#!/bin/bash
# Let's try a broader search for any usage of peers.data.connected
rg -A 3 "peers\.data\.connected" --type ts

# Also search for any references to this state path in the codebase
rg -A 3 "parsed\.connected" --type ts

Length of output: 863


Script:

#!/bin/bash
# Let's check the implementation to see how the connected peers data is used
ast-grep --pattern 'const $_ = $$.parsed.connected'

# Also check for any reducer actions that might modify this state
rg -A 5 "parsed\.connected.*=" --type ts

Length of output: 628

Comment on lines +34 to +44
const getAliasByPeerId = (peerId: string): string | JSX.Element => {
const shortPeerId = peerId && `${peerId.substring(0, 6)}...${peerId.substring(peerId.length - 8, peerId.length)}`;
const displayPeerId = props.shortenPeerId ? shortPeerId : peerId;
if (aliases && peerId && peerIdToAliasLink[peerId])
return (
<>
<strong>{peerIdToAliasLink[peerId]}</strong> ({displayPeerId})
</>
);
return displayPeerId;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation for peerId length

The current implementation assumes peerId is long enough to be shortened. This could cause issues with malformed or short peerIds.

Consider this safer implementation:

-    const shortPeerId = peerId && `${peerId.substring(0, 6)}...${peerId.substring(peerId.length - 8, peerId.length)}`;
+    const shortPeerId = peerId && peerId.length > 14
+      ? `${peerId.substring(0, 6)}...${peerId.substring(peerId.length - 8)}`
+      : peerId;
📝 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 getAliasByPeerId = (peerId: string): string | JSX.Element => {
const shortPeerId = peerId && `${peerId.substring(0, 6)}...${peerId.substring(peerId.length - 8, peerId.length)}`;
const displayPeerId = props.shortenPeerId ? shortPeerId : peerId;
if (aliases && peerId && peerIdToAliasLink[peerId])
return (
<>
<strong>{peerIdToAliasLink[peerId]}</strong> ({displayPeerId})
</>
);
return displayPeerId;
};
const getAliasByPeerId = (peerId: string): string | JSX.Element => {
const shortPeerId = peerId && peerId.length > 14
? `${peerId.substring(0, 6)}...${peerId.substring(peerId.length - 8)}`
: peerId;
const displayPeerId = props.shortenPeerId ? shortPeerId : peerId;
if (aliases && peerId && peerIdToAliasLink[peerId])
return (
<>
<strong>{peerIdToAliasLink[peerId]}</strong> ({displayPeerId})
</>
);
return displayPeerId;
};

@mjadach-iv mjadach-iv merged commit dca88b3 into main Dec 3, 2024
4 checks passed
@mjadach-iv mjadach-iv deleted the 2.1.12 branch December 3, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment