Fix: private key onboarding string #1142
Conversation
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (4)
components/Onboarding/PrivateKeyConnect.tsx (3)
Line range hint
44-54: Remove commented-out private key storage code.The commented code for private key storage contradicts the PR's objective of clarifying that private keys are not stored. To maintain code clarity and prevent potential confusion or accidental reactivation, this commented section should be removed.
} setSigner(signer); - // Let's save - // const pkPath = `PK-${uuidv4()}`; - // try { - // await savePrivateKey(pkPath, seedPhraseSigner.privateKey); - // setPkPath(pkPath); - // setSigner(seedPhraseSigner); - // } catch (e: any) { - // // No biometrics? - // Alert.alert("An error occured", e.toString()); - // setLoading(false); - // }
Line range hint
91-94: Enhance input validation and error handling.The current validation could be improved to provide better user feedback and maintain consistency.
primaryButtonText={translate("privateKeyConnect.connectButton")} primaryButtonAction={() => { - if (!privateKey || privateKey.trim().length === 0) return; - loginWithPrivateKey(privateKey.trim()); + const trimmedKey = privateKey.trim(); + if (!trimmedKey) { + Alert.alert(translate("privateKeyConnect.emptyKeyError")); + return; + } + loginWithPrivateKey(trimmedKey); }}
Line range hint
91-130: Consider UX improvements for private key input.A few suggestions to enhance the user experience:
- Add visual feedback during the loading state
- Consider using a single-line input with paste support, as private keys shouldn't contain line breaks
primaryButtonAction={() => { if (!privateKey || privateKey.trim().length === 0) return; loginWithPrivateKey(privateKey.trim()); }} backButtonText={translate("privateKeyConnect.backButton")} backButtonAction={() => { setConnectionMethod(undefined); }} showTerms + isLoading={loading} > <View style={styles.entryContainer}> <TextInput - multiline + multiline={false} textAlignVertical="top" style={styles.textInput} placeholder={translate("privateKeyConnect.privateKeyPlaceholder")}utils/frames.ts (1)
274-285: LGTM! Good refactoring ofisFrameMessage.The function has been improved by:
- Separating concerns with distinct parameters
- Adding clear content sanitization
- Implementing early returns
Consider these improvements for more robust URL handling:
export const isFrameMessage = ( messageIsText: boolean, messageContent: string, framesStore: { [frameUrl: string]: FrameWithType; } ): boolean => { if (!messageIsText) return false; + if (!messageContent) return false; const content = messageContent.toLowerCase().trim(); const sanitizedContent = content.endsWith("/") ? content.slice(0, -1) : content; + // Remove leading slash if present + const normalizedContent = sanitizedContent.startsWith("/") + ? sanitizedContent.slice(1) + : sanitizedContent; - return !!framesStore[sanitizedContent]; + return !!framesStore[normalizedContent]; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app.json(1 hunks)components/Chat/Message/Message.tsx(2 hunks)components/Onboarding/PrivateKeyConnect.tsx(1 hunks)i18n/translations/en.ts(1 hunks)utils/frames.ts(1 hunks)
🔇 Additional comments (6)
app.json (1)
5-5: LGTM! Version increments follow proper practices.
The version increments are consistent across platforms and follow proper versioning practices.
Let's verify if these version bumps align with the changes in the PR:
Also applies to: 8-8
✅ Verification successful
Version bumps are justified by significant UI/UX changes around private key handling
The version increments are warranted as the changes involve significant updates to the private key onboarding flow:
- Updates to user-facing messages in translations
- Changes to private key validation and error handling in EnableTransactions.tsx
- Modifications to the private key handling logic in multiple components
- Security-related messaging updates clarifying that private keys are not stored
These changes affect core functionality and user experience, justifying the version bumps across both platforms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for substantial changes that would warrant version bumps
# Look for changes in source files
echo "Checking for substantial changes in source files..."
rg -l --type-add 'src:*.{ts,tsx,js,jsx}' -t src 'private.*key'
# Look for changes in translation files
echo "Checking for changes in translation files..."
fd -e json -e ts translations/
Length of output: 432
Script:
#!/bin/bash
# Let's examine the actual changes in the identified files
# Check changes in files containing private key references
echo "Changes in EnableTransactions.tsx:"
rg -B 2 -A 2 'private.*key' screens/EnableTransactions.tsx
echo -e "\nChanges in api.ts:"
rg -B 2 -A 2 'private.*key' utils/xmtpRN/api.ts
echo -e "\nChanges in SendMoneyButton.tsx:"
rg -B 2 -A 2 'private.*key' components/Chat/Transaction/SendMoneyButton.tsx
echo -e "\nChanges in translations:"
rg -B 1 -A 1 'private.*key' i18n/translations/en.ts
Length of output: 1940
components/Onboarding/PrivateKeyConnect.tsx (1)
87-87: LGTM! Clear communication about private key handling.
The simplified subtitle effectively communicates that private keys are not stored, which aligns with the PR's objective to improve user understanding of private key handling.
utils/frames.ts (2)
274-285: Improved error handling in handleTxAction.
The error handling has been enhanced with:
- Early validation of required parameters
- Clear error messages for unsupported testnets
- Proper chain ID validation
- Transaction receipt verification
274-285: Verify PR objective alignment.
The PR description mentions fixing "private key onboarding string", but the changes in this file focus on frame message handling improvements. Please clarify if there are additional changes pending or if the PR description needs to be updated.
i18n/translations/en.ts (1)
66-66: LGTM! Clear and accurate security messaging.
The updated message "Your private key is not stored" provides a more direct and transparent communication about the application's private key handling policy. This change aligns well with the PR objective to accurately convey that the application does not store private keys.
Let's verify that all storage-related code has been removed:
✅ Verification successful
Verified: Private key storage code has been properly removed
The code inspection confirms that:
- The PrivateKeyConnect component no longer contains any storage-related functionality
- Previous storage-related code is commented out and appears to be in the process of removal
- The component only temporarily holds the private key in component state (
useState) for form handling - No localStorage, sessionStorage, or other persistent storage mechanisms are being used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to private key storage
# Test: Search for mentions of private key storage. Expect: No results indicating storage.
rg -i "private.?key.*stor(e|age|ed)" --type ts --type tsx
Length of output: 87
Script:
#!/bin/bash
# Search for private key storage references with correct file extensions
rg -i "private.?key.*stor(e|age|ed)" -t ts -t typescript -t js -t jsx
# Search for any storage-related code in PrivateKeyConnect component
rg -i "PrivateKeyConnect.*storage" -t ts -t typescript -t js -t jsx
# Find and examine PrivateKeyConnect component implementation
fd -e ts -e tsx -e js -e jsx | xargs rg "PrivateKeyConnect"
Length of output: 634
Script:
#!/bin/bash
# Examine the PrivateKeyConnect component implementation
cat components/Onboarding/PrivateKeyConnect.tsx
# Look for any storage-related code with simpler pattern
rg -i "storage" components/Onboarding/PrivateKeyConnect.tsx
# Check for any localStorage or sessionStorage usage
rg -i "localStorage|sessionStorage" components/Onboarding/PrivateKeyConnect.tsx
Length of output: 4641
components/Chat/Message/Message.tsx (1)
Line range hint 1-500: Implementation looks solid!
The code demonstrates good practices with:
- Efficient performance optimizations (memoization, caching)
- Proper error handling
- Clear separation of concerns
- Comprehensive feature implementation
| useShallow((s) => | ||
| isFrameMessage( | ||
| isContentType("text", message.contentType), | ||
| message.content, | ||
| s.frames | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing the content type check.
The isContentType check could be memoized outside the useFramesStore selector to avoid unnecessary re-computations, as it only depends on message.contentType.
Consider refactoring to:
+ const isTextContent = useMemo(
+ () => isContentType("text", message.contentType),
+ [message.contentType]
+ );
const isFrame = useFramesStore(
useShallow((s) =>
- isFrameMessage(
- isContentType("text", message.contentType),
- message.content,
- s.frames
- )
+ isFrameMessage(isTextContent, message.content, s.frames)
)
);Committable suggestion was skipped due to low confidence.
saulmc
force-pushed
the
saulmc/i18n-pk
branch
from
19232e9 to
1efc8b0
Compare
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
There was a problem hiding this comment.
Hey @saulmc I don't think your branch was based off 2.0.8 as the PrivateKeyConnect component has been removed by @thierryskoda !
There are now two components, OnboardingPrivateKeyScreen and NewAccountPrivateKeyScreen where the wording is used
| ios: "your iPhone's Secure Enclave", | ||
| android: "the Android Keystore system", | ||
| }, | ||
| "Enter the private key for the address you are connecting.\n\nYour private key is not stored.", |
There was a problem hiding this comment.
Just added this fix into my PR
There was a problem hiding this comment.
@saulmc Added the text fix into my PR.
Was the other stuff intended? Because it's brining back some deleted files.
And the changes on Message.tsx and frames.ts? I think it's related to what you were doing @alexrisch right?
|
Thanks @thierryskoda closing this! I don't know what's going on with the extra commits being added to the PR. Thought I screwed something up bc I hadn't made those changes locally. Fixed merge conflicts, pushed update, and they were back again. I noticed an action ran when I pushed -- wonder if that's possibly related... |
Fix to accurately state that we do not store private keys.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation