chalabi/safe truncation #204
Conversation
WalkthroughThe pull request introduces a series of changes across multiple components, focusing on enhancing the flexibility of chain configuration and address display. The primary modifications involve replacing hardcoded 'manifest' chain references with a dynamic Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Finishing Touches
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
components/groups/components/myGroups.tsx (1)
Line range hint
132-140: Standardize group name parsing logic.The group name parsing logic is duplicated and inconsistent between different parts of the code. Consider extracting this into a utility function for better maintainability and consistency.
Create a utility function:
function parseGroupName(metadata: string | null): string { if (!metadata) return 'Untitled Group'; try { const parsed = JSON.parse(metadata); return parsed?.title || 'Untitled Group'; } catch (e) { console.warn('Failed to parse group metadata:', e); return 'Untitled Group'; } }Then use it consistently throughout the code:
- try { - const metadata = group.metadata ? JSON.parse(group.metadata) : null; - groupName = metadata?.title ?? 'Untitled Group'; - } catch (e) { - // If JSON parsing fails, fall back to default name - // console.warn('Failed to parse group metadata:', e); - } + groupName = parseGroupName(group.metadata);Also applies to: 476-484
components/react/addressCopy.tsx (1)
Line range hint
4-11: Remove unused slice parameter and utilize truncateString utilityThe component accepts a
sliceparameter that's no longer used, and it duplicates truncation logic that exists in utils/string.ts.Consider this improvement:
- slice: number; size?: string; }) => { // ... - const truncatedAddress = `${address?.slice(0, 24)}...`; + const truncatedAddress = truncateString(address);Also applies to: 34-34
🧹 Nitpick comments (2)
components/groups/components/myGroups.tsx (1)
Line range hint
106-113: Restore error logging for better debuggability.The commented-out error logging reduces our ability to debug metadata parsing issues in production. Consider using a proper logging service or error tracking system instead of removing the logs.
} catch (e) { - // console.warn('Failed to parse group metadata:', e); + console.warn('Failed to parse group metadata:', e); + // TODO: Consider sending to error tracking service return 'Untitled Group'.toLowerCase().includes(searchTerm.toLowerCase()); }components/wallet.tsx (1)
Line range hint
1-1: Consider centralizing address truncation logicTo improve maintainability and consistency, consider:
- Making
truncateStringin utils/string.ts the single source of truth for address truncation- Adding a new
truncateAddressutility specifically for wallet addresses- Creating constants for common values like the truncation length
Example implementation:
// utils/constants.ts export const ADDRESS_TRUNCATE_LENGTH = 24; // utils/string.ts export function truncateAddress(address: string): string { return truncateString(address, ADDRESS_TRUNCATE_LENGTH); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
components/groups/components/groupControls.tsx(2 hunks)components/groups/components/myGroups.tsx(1 hunks)components/groups/forms/groups/ConfirmationForm.tsx(2 hunks)components/groups/modals/voteModal.tsx(2 hunks)components/react/addressCopy.tsx(1 hunks)components/wallet.tsx(1 hunks)utils/string.ts(1 hunks)
🔇 Additional comments (4)
components/groups/modals/voteModal.tsx (1)
7-7: LGTM! Chain configuration changes look good.The replacement of hardcoded chain name with environment configuration improves flexibility and maintainability.
Also applies to: 17-19
components/groups/forms/groups/ConfirmationForm.tsx (1)
9-9: LGTM! Chain configuration changes look good.The replacement of hardcoded chain name with environment configuration improves flexibility and maintainability.
Also applies to: 33-34
components/groups/components/groupControls.tsx (1)
29-29: LGTM! Chain configuration changes look good.The replacement of hardcoded chain name with environment configuration improves flexibility and maintainability.
Also applies to: 235-235
components/groups/components/myGroups.tsx (1)
103-103: LGTM! Chain configuration changes look good.The replacement of hardcoded chain name with environment configuration improves flexibility and maintainability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
==========================================
+ Coverage 54.24% 54.39% +0.14%
==========================================
Files 153 153
Lines 15982 15960 -22
==========================================
+ Hits 8670 8681 +11
+ Misses 7312 7279 -33 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
components/groups/forms/groups/__tests__/Success.test.tsx (1)
26-28: Strengthen test assertions for address truncation.The current test assertions only verify the prefix of truncated addresses. Consider using exact string matching to ensure the truncation behavior is precisely as expected.
- expect(screen.getByText('manifest1autho...')).toBeInTheDocument(); - expect(screen.getByText('manifest1efd63...')).toBeInTheDocument(); - expect(screen.getByText('manifest1hj5fv...')).toBeInTheDocument(); + expect(screen.getByText('manifest1autho...', { exact: false })).toBeInTheDocument(); + const addresses = screen.getAllByText(/^manifest1[a-z0-9]+\.{3}$/); + expect(addresses).toHaveLength(3);components/react/addressCopy.tsx (1)
3-3: LGTM! Consider adding error boundary.The refactoring to use the
truncateAddressutility and adding a default value for the address prop improves code maintainability and robustness.Consider wrapping the clipboard operations in an error boundary to gracefully handle cases where the clipboard API is not available:
+const useClipboard = () => { + const copyToClipboard = async (text: string) => { + if (!navigator.clipboard) { + throw new Error('Clipboard API not available'); + } + return navigator.clipboard.writeText(text); + }; + return { copyToClipboard }; +}; export const TruncatedAddressWithCopy = ({ address = '', slice, size, }: { address: string; slice: number; size?: string; }) => { const [copied, setCopied] = useState(false); + const { copyToClipboard } = useClipboard(); // ... rest of the component const handleCopy = async (e: React.MouseEvent<HTMLDivElement>) => { e.stopPropagation(); e.preventDefault(); try { - await navigator.clipboard.writeText(address); + await copyToClipboard(address); setCopied(true); } catch (err) { console.error('Failed to copy: ', err); + // Optionally show a user-friendly error message } };Also applies to: 6-6, 35-35
utils/string.ts (2)
3-7: Add input validation to prevent edge cases.The function should validate its inputs to handle edge cases gracefully.
export function truncateString(str: string, prefixLen: number = 6, suffixLen: number = 6): string { + if (!str) return ''; + if (prefixLen < 0 || suffixLen < 0) return str; if (str.length <= prefixLen + suffixLen) return str; return str.slice(0, prefixLen) + '...' + str.slice(-suffixLen); }
9-12: Add address validation and format checking.The function should validate the input address and its format.
export function truncateAddress(address: string, num: number = 24) { + if (!address) return ''; + if (num < 0) return address; + // Optional: Add address format validation if needed + // if (!isValidManifestAddress(address)) return address; if (address.length <= num) return address; return address.slice(0, num) + '...'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
components/admins/components/validatorList.tsx(1 hunks)components/bank/components/__tests__/historyBox.test.tsx(2 hunks)components/bank/components/historyBox.tsx(1 hunks)components/bank/modals/txInfo.tsx(1 hunks)components/groups/components/myGroups.tsx(2 hunks)components/groups/forms/groups/ConfirmationForm.tsx(2 hunks)components/groups/forms/groups/__tests__/Success.test.tsx(1 hunks)components/groups/modals/memberManagementModal.tsx(2 hunks)components/react/addressCopy.tsx(2 hunks)components/react/views/Connected.tsx(2 hunks)components/wallet.tsx(2 hunks)utils/string.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/groups/components/myGroups.tsx
🧰 Additional context used
🪛 GitHub Check: codecov/patch
components/bank/components/historyBox.tsx
[warning] 251-251: components/bank/components/historyBox.tsx#L251
Added line #L251 was not covered by tests
components/react/views/Connected.tsx
[warning] 86-86: components/react/views/Connected.tsx#L86
Added line #L86 was not covered by tests
components/groups/forms/groups/ConfirmationForm.tsx
[warning] 34-35: components/groups/forms/groups/ConfirmationForm.tsx#L34-L35
Added lines #L34 - L35 were not covered by tests
components/wallet.tsx
[warning] 134-134: components/wallet.tsx#L134
Added line #L134 was not covered by tests
[warning] 138-138: components/wallet.tsx#L138
Added line #L138 was not covered by tests
🔇 Additional comments (5)
components/bank/components/__tests__/historyBox.test.tsx (1)
10-17: LGTM! Clean router mock implementation.The router mock is well-implemented with proper cleanup in the afterEach block.
components/bank/modals/txInfo.tsx (1)
120-120: LGTM! Consistent address truncation length.The update to use 24 characters for address truncation aligns with the standardization effort across the application.
components/admins/components/validatorList.tsx (1)
229-229: LGTM!The change to standardize address truncation to 24 characters is consistent with the application-wide update. The component maintains good accessibility with proper ARIA attributes.
components/groups/modals/memberManagementModal.tsx (1)
14-14: Verify form submission with truncated address display.While the UI change to display truncated addresses is consistent with the application-wide update, ensure that the form submission still uses the full address value.
Let's verify the form submission behavior:
Also applies to: 325-325
✅ Verification successful
Form submission correctly uses full addresses
The truncateAddress function is only used for display purposes in the input field UI. The form submission logic in handleConfirm uses the raw, untruncated addresses from the form state when building the transaction messages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the full address is preserved during form submission # Test: Search for form submission handling code ast-grep --pattern 'handleConfirm = async (values: { members: ExtendedMember[] }) => { $$$ const encodedMessages = buildMessages(values.members); $$$ }'Length of output: 160
Script:
#!/bin/bash # Find usage of truncateAddress rg "truncateAddress" -A 5 # Look for form submission patterns ast-grep --pattern 'const $_ = async ($_) => { $$$ }' # Also search for form submission related terms rg "onSubmit|handleSubmit|formik" -A 5Length of output: 66321
components/groups/forms/groups/ConfirmationForm.tsx (1)
38-39: LGTM! Good use of environment configuration.The replacement of hardcoded chain name with
env.chainimproves flexibility and maintainability.
| @@ -5,7 +5,7 @@ | |||
| import ProfileAvatar from '@/utils/identicon'; | |||
| import { useBalance } from '@/hooks/useQueries'; | |||
| import { CopyIcon } from '@/components/icons'; | |||
| import { getRealLogo, shiftDigits, truncateString } from '@/utils'; | |||
| import { getRealLogo, shiftDigits, truncateAddress } from '@/utils'; | |||
There was a problem hiding this comment.
💡 Codebase verification
Add basic test coverage for truncateAddress utility
The truncateAddress function in utils/string.ts lacks test coverage. While the implementation is straightforward, tests should verify:
- Handling of addresses shorter than the limit
- Truncation of addresses longer than the default 24 characters
- Empty string handling
🔗 Analysis chain
Add test coverage for address truncation.
The address truncation implementation looks good, but the static analysis indicates that line 86 lacks test coverage. Consider adding test cases to verify the truncation behavior.
Would you like me to help generate test cases for the address truncation functionality?
Also applies to: 86-86
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing tests of Connected component
fd -e tsx -e ts "Connected.test" components/react/views/
Length of output: 56
Script:
#!/bin/bash
# Find all test files in views directory
echo "=== Test files in views ==="
fd -e test.tsx -e test.ts . components/react/views/
# Find truncateAddress tests
echo -e "\n=== truncateAddress tests ==="
fd -e test.tsx -e test.ts . utils/
# Find actual usage of truncateAddress
echo -e "\n=== truncateAddress usage in Connected ==="
rg -C 2 "truncateAddress" components/react/views/Connected.tsx
Length of output: 848
Script:
#!/bin/bash
# Find truncateAddress tests using ripgrep
echo "=== Searching for truncateAddress tests ==="
rg -l "truncateAddress.*test"
# Find truncateAddress implementation
echo -e "\n=== truncateAddress implementation ==="
rg -C 4 "export.*truncateAddress"
Length of output: 628
| @@ -248,7 +248,7 @@ | |||
| ? tx.data.to_address | |||
| : tx.data.from_address | |||
| } | |||
| slice={6} | |||
| slice={24} | |||
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for address truncation behavior
The existing test suite in components/bank/components/__tests__/historyBox.test.tsx lacks coverage for the address truncation functionality. Consider adding a test case that verifies addresses are correctly truncated to 24 characters.
🔗 Analysis chain
LGTM! Consider adding test coverage.
The change to standardize address truncation to 24 characters aligns with the broader effort to ensure consistent address display across the application.
The line is flagged as not covered by tests. Consider adding test coverage to ensure the address truncation behavior works as expected:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing test files and patterns
# Test: Search for test files that might need updating
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts | rg -i "historybox|address"
Length of output: 136
Script:
#!/bin/bash
# Check the content of historyBox test file for address-related test cases
cat components/bank/components/__tests__/historyBox.test.tsx
Length of output: 3679
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 251-251: components/bank/components/historyBox.tsx#L251
Added line #L251 was not covered by tests
| let jsonString: string; | ||
| try { | ||
| jsonString = JSON.stringify(groupMetadata); | ||
| } catch (error) { | ||
| console.error('Failed to serialize group metadata:', error); | ||
| throw new Error('Invalid group metadata format'); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for JSON serialization error handling
The test file lacks coverage for the JSON serialization error handling. Consider adding these test cases:
- Test successful serialization of valid group metadata
- Test error handling when serialization fails (e.g., with circular references)
🔗 Analysis chain
Add test coverage for error handling.
The error handling for JSON serialization is good, but it lacks test coverage.
Could you add test cases to cover the error handling scenario? Here's a verification script to check current test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for the error handling block
rg -l "test.*ConfirmationForm.*metadata.*serialization" -t ts
Length of output: 63
Script:
#!/bin/bash
# Find test files related to ConfirmationForm
fd "ConfirmationForm.*test" --type f
# If no direct test file found, check for test files in the same directory
fd ".*test.*\.(ts|tsx)$" --type f --exec grep -l "ConfirmationForm"
Length of output: 366
Script:
#!/bin/bash
# Check content of the relevant test file
cat "components/groups/forms/groups/__tests__/ConfirmationForm.test.tsx"
Length of output: 2752
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-35: components/groups/forms/groups/ConfirmationForm.tsx#L34-L35
Added lines #L34 - L35 were not covered by tests
| ? `${username.slice(0, 20)}...` | ||
| : username | ||
| : 'Connected User'} | ||
| {username ? truncateString(username, 20) : 'Connected User'} |
There was a problem hiding this comment.
💡 Codebase verification
Add comprehensive test coverage for wallet component and utilities
The verification confirms that test coverage is missing for:
- The wallet component functionality
- The
truncateStringutility function - Username truncation behavior
Please add test cases covering both the component behavior and the utility function, ensuring various scenarios like:
- Username truncation with different lengths
- Empty username handling
- Edge cases for the truncation function
🔗 Analysis chain
Add test coverage for username truncation.
The username truncation implementation lacks test coverage.
Could you add test cases for the username truncation? Here's a verification script to check current test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for username truncation
rg -l "test.*WalletSection.*username.*truncate" -t ts
Length of output: 55
Script:
#!/bin/bash
# Find all test files related to wallet
echo "=== Wallet-related test files ==="
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts | rg -i "wallet"
# Find truncateString utility tests
echo -e "\n=== TruncateString utility tests ==="
rg -l "truncateString" -t ts | rg "test|spec"
# Find any username truncation tests
echo -e "\n=== Username truncation tests ==="
rg -l "username.*truncate|truncate.*username" -t ts
Length of output: 436
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 134-134: components/wallet.tsx#L134
Added line #L134 was not covered by tests
| {address | ||
| ? `${address.slice(0, 12)}...${address.slice(-6)}` | ||
| : 'Address not available'} | ||
| {address ? truncateAddress(address) : 'Address not available'} |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for address truncation utility
The address truncation functionality requires test coverage at two levels:
- Unit tests for the
truncateAddressutility function in the utils directory - Integration tests for address display in the WalletSection component
Example test cases should cover:
- Different address lengths
- Edge cases (empty string, null, undefined)
- Correct truncation pattern
🔗 Analysis chain
Add test coverage for address truncation.
The address truncation implementation lacks test coverage.
Could you add test cases for the address truncation? Here's a verification script to check current test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for address truncation
rg -l "test.*WalletSection.*address.*truncate" -t ts
Length of output: 54
Script:
#!/bin/bash
# Find truncateAddress function definition
ast-grep --pattern 'function truncateAddress($_) { $$$ }'
# Find all test files related to wallet
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts . | rg -i "wallet"
# Search for any tests containing "address" string
rg "test.*address" -t ts -t tsx
Length of output: 189
Script:
#!/bin/bash
# Find all wallet-related files to understand structure
fd . -e ts -e tsx | rg -i "wallet"
# Try different patterns for truncateAddress
ast-grep --pattern 'const truncateAddress = $_'
ast-grep --pattern 'export const truncateAddress = $_'
ast-grep --pattern 'export function truncateAddress($_)'
# Search for address-related tests with correct extension
rg "test.*address" -t ts
Length of output: 2538
Script:
#!/bin/bash
# Check for utility files and inline truncate implementation
rg "truncate.*address" -t ts -t tsx
# Look at the wallet component implementation
cat components/wallet.tsx
Length of output: 10539
Script:
#!/bin/bash
# Check for utils tests
fd . -e test.ts -e test.tsx | rg "utils"
# Look at the utils implementation
cat utils/index.ts
Length of output: 400
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 138-138: components/wallet.tsx#L138
Added line #L138 was not covered by tests
Fixes #200
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores
Successcomponent to focus on partial string matches.