chore: fix types #1073
chore: fix types #1073
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
technoplato
force-pushed
the
ml/10-22-refactor_use_state_machine_for_logic_in_screen
branch
from
daf32c2 to
ec3d7e8
Compare
technoplato
force-pushed
the
ml/10-22-chore_fix_types
branch
from
46ae8ca to
d716df7
Compare
technoplato
force-pushed
the
ml/10-22-refactor_use_state_machine_for_logic_in_screen
branch
from
ec3d7e8 to
85040ca
Compare
technoplato
force-pushed
the
ml/10-22-chore_fix_types
branch
from
d716df7 to
42c8f99
Compare
technoplato
force-pushed
the
ml/10-22-refactor_use_state_machine_for_logic_in_screen
branch
from
85040ca to
81e9f33
Compare
technoplato
force-pushed
the
ml/10-22-chore_fix_types
branch
from
42c8f99 to
d8d4bfb
Compare
technoplato
force-pushed
the
ml/10-22-refactor_use_state_machine_for_logic_in_screen
branch
from
81e9f33 to
225b3b9
Compare
technoplato
force-pushed
the
ml/10-22-chore_fix_types
branch
from
d8d4bfb to
0d5fb74
Compare
* fix: Repeated messages showing as scaled and lower opacity Set default message animation Updated check to be a ref instead Made animation a worklet * Update animation, block potential race conditions
* Handle resolution error & timeout for deeplink * Typings and some rabbit remarks * Fix typings
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
* fix: Render Frame on initial send Fixes rendering from on initial send Cleans up types Updates utils * fix ratio types
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
* fix: URL Crash Added safety to URL Reverted to FramesStore access for full frame * fix: URL Crashes Reverted change to set isFrames Added additional safety Attempted some improved performance
* fix: URL Crash Added safety to URL Reverted to FramesStore access for full frame * fix: URL Crashes Reverted change to set isFrames Added additional safety Attempted some improved performance * [create-pull-request] automated change --------- Co-authored-by: Alex Risch <alex.j.risch@gmail.com> Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
* Fabri QA for frames & deeplinks * also allow ' & " at beginning * comments
Added Branching Docs
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
* fix: Frames Renders on initial Send Updated to use zustand store for managing frames Updated to use hasFrames rather than isFrame * cleanup
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
Added handling to clean url Updated to use util function
* Allow ethereum deeplinks inside frames * Set and new file * Bump app json * [create-pull-request] automated change (#1195) Co-authored-by: alexrisch <alexrisch@users.noreply.github.com> * [create-pull-request] automated change (#1197) Co-authored-by: alexrisch <alexrisch@users.noreply.github.com> --------- Co-authored-by: Noé Malzieu <noe.malzieu@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: alexrisch <alexrisch@users.noreply.github.com>
…aHQ/converse-app into ml/10-22-chore_fix_types
WalkthroughThe pull request introduces multiple changes across various files, primarily focusing on enhancing the handling of frames in chat messages, updating configuration files, and improving documentation. Key updates include modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatComponent
participant FrameStore
participant MessageUtils
User->>ChatComponent: Send Message
ChatComponent->>FrameStore: Fetch Frames
FrameStore-->>ChatComponent: Return Frames
ChatComponent->>MessageUtils: Process Message
MessageUtils-->>ChatComponent: Return Processed Message
ChatComponent-->>User: Display Message
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 (17)
utils/messageContent.ts (1)
3-9: Good error handling improvement, consider adding input validation.The addition of proper error handling and explicit return type is a good improvement. However, consider adding input validation to handle empty or whitespace-only URLs.
Consider this enhancement:
export const getUrlToRender = (url: string): string | undefined => { + if (!url?.trim()) { + return undefined; + } try { const fullUrl = new URL(url); return fullUrl.hostname; } catch { return undefined; } };data/store/framesStore.ts (2)
38-44: Simplify state update by removing redundant spread ofstateIn Zustand, the updater function merges the returned partial state with the current state. Spreading
...stateis unnecessary and can be omitted for cleaner code.Apply this diff to simplify the state update:
return { - ...state, frames: { ...state.frames, ...framesToSet }, messageFramesMap: { ...state.messageFramesMap, [messageId]: Object.values(framesToSet), }, };
60-65: Remove unnecessary spread ofstatein state updaterThe spread operator
...stateis not needed since Zustand automatically merges the returned object with the current state.Apply this diff to clean up the state updater:
return { - ...state, messageFramesMap: { ...state.messageFramesMap, [messageId]: frames, }, };utils/xmtpRN/send.ts (1)
Line range hint
63-63: Correct control flow to avoid exiting the function prematurelyUsing
returninside a loop will exit the entire function instead of continuing to the next iteration. Usecontinueto skip the current iteration when conditions are met.Apply this diff to fix the loop control flow:
if (sendingMessages[id] || !preparedMessage.topic) { - return; + continue; }utils/notifications.ts (1)
Line range hint
239-246: Ensure proper handling of notification data parsingIn the
onInteractWithNotificationfunction, the parsing logic fornotificationDatamay not handle all possible data structures, potentially leading to runtime errors.Consider adding additional checks to ensure that
notificationDatais correctly parsed and to handle cases where expected properties may be missing or of unexpected types.utils/xmtpRN/conversations.ts (1)
693-693: Check forgroup.syncerrors and handle exceptionsAfter calling
await group.sync();, it's good practice to handle potential exceptions to prevent unhandled promise rejections.Wrap the
group.sync()call in a try-catch block to gracefully handle any errors:+ try { await group.sync(); + } catch (error) { + logger.error(`Error syncing group ${topic}:`, error); + }components/Chat/Frame/urlProtocols.ts (1)
3-6: Consider adding validation for URL protocolsWhile the implementation is clean and efficient, consider adding validation to ensure that
config.framesAllowedSchemescontains valid URL protocol strings.export const AUTHORIZED_URL_PROTOCOLS = new Set([ `${config.scheme}:`, - ...config.framesAllowedSchemes.map((s) => `${s}:`), + ...config.framesAllowedSchemes + .filter((s) => /^[a-zA-Z][a-zA-Z0-9+.-]*$/.test(s)) + .map((s) => `${s}:`), ]);utils/evm/address.ts (1)
Line range hint
9-24: Fix duplicated condition in isSupportedPeerThere's a duplicated
isENScondition in the return statement which should be removed.return ( isUserName || is0x || isLens || isENS || - isENS || isFarcaster || isCbId || isUNS );components/Chat/Frame/FramesPreviews.tsx (2)
30-39: Consider adding error handling for frame fetchingThe frame fetching logic should include error handling to gracefully handle failed requests.
fetchFramesForMessage(account, message).then( (frames: FramesForMessage) => { setMessageFramesMap(frames.messageId, frames.frames); - } + } +).catch((error) => { + console.error('Failed to fetch frames:', error); + // Consider showing a user-friendly error state );
1-10: Consider grouping related importsThe imports could be better organized by grouping related imports together (external libraries, internal utilities, components, etc.).
-import { useSelect } from "@data/store/storeHelpers"; -import { useConversationContext } from "@utils/conversation"; -import { useCallback, useEffect, useRef } from "react"; -import { View } from "react-native"; -import { useShallow } from "zustand/react/shallow"; - -import FramePreview from "./FramePreview"; -import { useCurrentAccount } from "../../../data/store/accountsStore"; -import { useFramesStore } from "../../../data/store/framesStore"; -import { FramesForMessage, fetchFramesForMessage } from "../../../utils/frames"; +// React and React Native +import { useCallback, useEffect, useRef } from "react"; +import { View } from "react-native"; + +// External Libraries +import { useShallow } from "zustand/react/shallow"; + +// Internal Utilities +import { useSelect } from "@data/store/storeHelpers"; +import { useConversationContext } from "@utils/conversation"; +import { useCurrentAccount } from "../../../data/store/accountsStore"; +import { useFramesStore } from "../../../data/store/framesStore"; +import { FramesForMessage, fetchFramesForMessage } from "../../../utils/frames"; + +// Components +import FramePreview from "./FramePreview";README.md (1)
104-143: Improve documentation formatting and styleThe new Release Processes section is valuable, but there are a few improvements to consider:
- Change heading level from h3 to h2 for "Release Processes" to maintain proper hierarchy
- Consider rewording phrases with "off of" to be more concise (e.g., "based on" instead of "based off of")
- Use "when" instead of "where" in the Exceptions section
Apply these changes:
-### Release Processes +## Release Processes ### Main Branch Represents the current production code. ### Release Branches -Each release branch is based off of `main` or the release branch before it. +Each release branch is based on `main` or the release branch before it. ### Feature Branches Feature branches are longer-lived features or refactors expected to take additional time. They should be based -off of +on the targeted feature release branch. ### Exceptions -There are certain times where this flow does not work as intended. +There are certain times when this flow does not work as intended.🧰 Tools
🪛 LanguageTool
[style] ~110-~110: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...e Branches Each release branch is based off ofmainor the release branch before it....(EN_WORDINESS_PREMIUM_OFF_OF)
[grammar] ~112-~112: This phrase is duplicated. You should probably use “Feature Branches” only once.
Context: ...e version (e.g.,release/2.0.8). ### Feature Branches Feature branches are longer-lived features or refactors ...(PHRASE_REPETITION)
[style] ~113-~113: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...e additional time. They should be based off of the targeted feature release branch. T...(EN_WORDINESS_PREMIUM_OFF_OF)
[style] ~140-~140: It might be better to use ‘times’ with the time-relative pronoun ‘when’. (Alternatively, use ‘in/on which’.)
Context: ... ``` ### Exceptions There are certain times where this flow does not work as intended. Fo...(WHEN_WHERE)
[style] ~141-~141: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...Build scripts: These may need to be run off of the default main branch instead of feat...(EN_WORDINESS_PREMIUM_OFF_OF)
🪛 Markdownlint (0.35.0)
106-106: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time(MD001, heading-increment)
components/Chat/ChatPlaceholder/GroupChatPlaceholder.tsx (1)
79-85: Enhance error message clarity with additional contextWhile the current implementation is good, consider adding more specific information in the error messages:
- {topic - ? isGroupTopic(topic) - ? translate("group_not_found") - : translate("conversation_not_found") - : translate("opening_conversation")} + {topic + ? isGroupTopic(topic) + ? translate("group_not_found", { topic }) + : translate("conversation_not_found", { topic }) + : translate("opening_conversation")}data/db/index.ts (1)
Line range hint
87-95: Consider enhancing error tracking for migrations.While the migration error handling is good, consider adding more detailed logging of the migration results to help with debugging.
- logger.debug(migrationsResult); + logger.debug('Migration results:', { + account, + migrationsApplied: migrationsResult.length, + migrations: migrationsResult.map(m => m.name) + });utils/logout/index.tsx (1)
Line range hint
136-152: Consider enhancing error handling in logout tasks.The error handling in the logout tasks could be more specific about the types of errors that can occur.
} catch (e: any) { if (e.toString().includes("CONVERSE_ACCOUNT_LOGGED_IN")) { removeLogoutTask(account); } else { - logger.error(e, { - context: `[Logout] Could not finish logging out for ${account}`, - }); + logger.error(e, { + context: `[Logout] Could not finish logging out for ${account}`, + account, + task: tasks[account], + errorType: e.name, + errorMessage: e.message + }); } }components/Chat/Message/MessageActions.tsx (1)
371-393: Well-structured animation configuration.The animation logic has been improved with:
- Clear separation of timing and spring configurations
- Better organization of animation parameters
- Proper worklet annotations
Consider extracting the animation configs into constants:
+const TIMING_CONFIG = { + duration: 250, + easing: Easing.inOut(Easing.quad), +}; + +const SPRING_CONFIG = { + damping: 10, + stiffness: 200, + mass: 0.2, + overshootClamping: false, + restSpeedThreshold: 0.001, + restDisplacementThreshold: 0.001, +}; const animateIn = useCallback(() => { "worklet"; opacity.value = 0; scale.value = 0.7; translateY.value = 20; - const timingConfig = { - duration: 250, - easing: Easing.inOut(Easing.quad), - }; - const springConfig = { - damping: 10, - stiffness: 200, - mass: 0.2, - overshootClamping: false, - restSpeedThreshold: 0.001, - restDisplacementThreshold: 0.001, - }; - opacity.value = withTiming(1, timingConfig); - scale.value = withSpring(1, springConfig); - translateY.value = withSpring(0, springConfig); + opacity.value = withTiming(1, TIMING_CONFIG); + scale.value = withSpring(1, SPRING_CONFIG); + translateY.value = withSpring(0, SPRING_CONFIG); }, [opacity, scale, translateY]);utils/conversation.ts (1)
203-221: Improve timeout handling with AbortControllerThe timeout implementation for identity resolution is good, but could be enhanced using AbortController for better cleanup.
Consider using AbortController for a more robust timeout implementation:
+const abortController = new AbortController(); +const timeoutId = setTimeout(() => { + abortController.abort(); if (!isDone) { isDone = true; Alert.alert( translate("identity_not_found_title"), translate("identity_not_found_timeout", { identity: peerToCreateConvoWith, }), [{ text: "OK", onPress: onError, isPreferred: true, }] ); } }, 5000); + +// Cleanup timeout +return () => { + clearTimeout(timeoutId); + abortController.abort(); +};components/Chat/Message/Message.tsx (1)
235-239: Improved URL handling with validationThe URL handling could be enhanced with additional validation.
Consider adding URL validation:
const handleUrlPress = useCallback((url: string) => { const cleanedUrl = url.toLowerCase().trim(); + const urlPattern = /^(https?:\/\/)?([\da-z.-]+)\.([a-z.]{2,6})([/\w .-]*)*\/?$/; + + if (!urlPattern.test(cleanedUrl)) { + Alert.alert('Invalid URL', 'Please enter a valid URL'); + return; + } const uri = cleanedUrl.startsWith("http") ? cleanedUrl : `https://${cleanedUrl}`; Linking.openURL(uri); }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
docs/image.pngis excluded by!**/*.png
📒 Files selected for processing (33)
.gitignore(1 hunks)README.md(1 hunks)app.json(1 hunks)components/Chat/Chat.tsx(7 hunks)components/Chat/ChatPlaceholder/GroupChatPlaceholder.tsx(3 hunks)components/Chat/Frame/FramePreview.tsx(3 hunks)components/Chat/Frame/FramesPreviews.tsx(1 hunks)components/Chat/Frame/urlProtocols.ts(1 hunks)components/Chat/Message/Message.tsx(14 hunks)components/Chat/Message/MessageActions.tsx(5 hunks)config.ts(1 hunks)data/db/index.ts(1 hunks)data/helpers/messages/handleGroupUpdatedMessage.test.ts(1 hunks)data/helpers/messages/handleGroupUpdatedMessage.ts(1 hunks)data/store/framesStore.ts(2 hunks)i18n/translations/en.ts(1 hunks)screens/Conversation.tsx(3 hunks)utils/conversation.ts(4 hunks)utils/events.ts(1 hunks)utils/evm/address.ts(1 hunks)utils/evm/xmtp.ts(1 hunks)utils/frames.ts(2 hunks)utils/logout/index.tsx(1 hunks)utils/messageContent.ts(1 hunks)utils/notifications.ts(1 hunks)utils/regex.ts(1 hunks)utils/result.type.ts(1 hunks)utils/xmtpRN/attachments.ts(1 hunks)utils/xmtpRN/client.ts(2 hunks)utils/xmtpRN/conversations.ts(2 hunks)utils/xmtpRN/messages.ts(1 hunks)utils/xmtpRN/send.ts(1 hunks)utils/xmtpRN/sync.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- app.json
- utils/xmtpRN/messages.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~110-~110: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...e Branches Each release branch is based off of main or the release branch before it....
(EN_WORDINESS_PREMIUM_OFF_OF)
[grammar] ~112-~112: This phrase is duplicated. You should probably use “Feature Branches” only once.
Context: ...e version (e.g., release/2.0.8). ### Feature Branches Feature branches are longer-lived features or refactors ...
(PHRASE_REPETITION)
[style] ~113-~113: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...e additional time. They should be based off of the targeted feature release branch. T...
(EN_WORDINESS_PREMIUM_OFF_OF)
[style] ~140-~140: It might be better to use ‘times’ with the time-relative pronoun ‘when’. (Alternatively, use ‘in/on which’.)
Context: ... ``` ### Exceptions There are certain times where this flow does not work as intended. Fo...
(WHEN_WHERE)
[style] ~141-~141: ‘off of’ might be wordy. Consider a shorter alternative.
Context: ...Build scripts: These may need to be run off of the default main branch instead of feat...
(EN_WORDINESS_PREMIUM_OFF_OF)
🪛 Markdownlint (0.35.0)
README.md
106-106: Expected: h2; Actual: h3
Heading levels should only increment by one level at a time
(MD001, heading-increment)
🔇 Additional comments (32)
utils/messageContent.ts (1)
3-9: Verify handling of undefined return value.
Since the function now explicitly returns undefined on error, we should ensure all calling code properly handles this case.
✅ Verification successful
Let me analyze the findings:
-
There are two implementations of
getUrlToRender:- In
utils/messageContent.ts(new version with undefined) - In
utils/xmtpRN/messages.ts(old version without undefined)
- In
-
The only actual usage in application code is in
components/Chat/Message/Message.tsxwhere it's used within a Text component.
Let me verify if there's proper type checking or error handling around the usage:
Let me check one more thing - the condition that controls when this URL rendering happens:
The undefined return value is safely handled in the codebase
The getUrlToRender function is only used in a context where its undefined return value is implicitly safe:
- It's only rendered within a conditional block
isFrame &&which means the URL has already been validated as a frame - The URL is displayed in a Text component which safely handles undefined by rendering nothing
- The tests comprehensively cover the URL parsing functionality
- There's no direct property access on the return value that could cause runtime errors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getUrlToRender to verify they handle undefined
# Search for function usage
echo "Searching for getUrlToRender usage..."
rg -l "getUrlToRender" | while read -r file; do
echo "=== $file ==="
# Show the usage with context
rg -C 5 "getUrlToRender" "$file"
done
# Search for potential direct property access on the result
echo "\nChecking for potential unsafe property access..."
rg "getUrlToRender\(.+\)\."
Length of output: 5188
Script:
#!/bin/bash
# Get more context around the usage in Message.tsx
ast-grep --pattern 'getUrlToRender($$$)' components/Chat/Message/Message.tsx
# Check if there's any conditional rendering or error handling
rg -B 10 -A 2 "getUrlToRender\(message\.content\)" components/Chat/Message/Message.tsx
Length of output: 914
Script:
#!/bin/bash
# Find where isFrame is defined/checked
rg -B 15 -A 2 "isFrame &&" components/Chat/Message/Message.tsx
# Also check for any URL validation
rg -B 5 "handleUrlPress" components/Chat/Message/Message.tsx
Length of output: 2735
data/store/framesStore.ts (1)
37-37:
Prevent potential state overwrite by returning undefined instead of an empty object
Returning an empty object {} from the set function's updater may overwrite the entire state with {}. To avoid unintended state changes, return undefined when no state update is needed.
Apply this diff to fix the issue:
if (isDeepEqual(existingFrames, framesToSet)) {
- return {};
+ return undefined;
}Likely invalid or redundant comment.
utils/result.type.ts (1)
1-203: Well-implemented Result class for robust error handling
The Result class provides a clean and type-safe way to handle operations that may fail, including helpful methods like map and flatMap for chaining operations.
utils/evm/xmtp.ts (1)
3-3: LGTM! Import path update aligns with type refactoring
The change correctly updates the import path for ConverseXmtpClientType as part of the type refactoring effort.
utils/evm/address.ts (1)
54-54: LGTM! Explicit undefined return improves type safety
The change to return undefined explicitly makes the function's behavior more predictable and TypeScript-friendly.
utils/regex.ts (1)
1-4: Verify the updated boundary regex patterns
The expanded boundary patterns now include quotes and commas, which improves text matching capabilities. However, let's verify there are no unintended side effects with these patterns.
✅ Verification successful
Let me gather more context about how these regex patterns are used in the codebase.
Let me check how these patterns are actually used in frames.ts to understand the impact of the boundary changes.
Let me try one final verification to understand the usage in frames.ts.
Based on the comprehensive analysis of the codebase, I can now provide the final response:
The updated boundary regex patterns are safe to use ✅
The changes to include quotes and commas in the boundary patterns are well-designed and safe because:
- The regex patterns are only used in
getDomainExpression()which creates specific domain-matching patterns (.lens,.eth,.fc, etc.) - The only direct usage is through
URL_REGEXinframes.tsfor URL extraction from messages - The boundary changes actually improve the accuracy of domain detection by properly handling URLs within quotes or followed by commas
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for existing usage of these regex patterns to ensure compatibility
# Look for potential test files or usage examples
# Search for test files
fd -e test.ts -e test.tsx -x grep -l "BOUNDARY_START|BOUNDARY_END"
# Search for usage of these patterns
rg "BOUNDARY_(START|END)" -A 3
Length of output: 1092
Script:
#!/bin/bash
# Search for test files that might contain test cases for these patterns
rg -l "getDomainExpression|WORD_CONTENT" "**/*.test.ts" "**/*.spec.ts"
# Look for actual usage of these patterns to understand the impact
ast-grep --pattern 'const $_ = getDomainExpression($_)'
# Search for any imports of regex.ts to understand usage scope
rg -l "from ['\"]\./regex['\"]"
Length of output: 803
Script:
#!/bin/bash
# Check how these regex patterns are used in frames.ts
rg "LENS_REGEX|ETH_REGEX|FARCASTER_REGEX|CB_ID_REGEX|UNS_REGEX" utils/frames.ts -A 2 -B 2
# Look for any test files that might verify these regex patterns
fd "frames.test" -e ts -e tsx
Length of output: 121
Script:
#!/bin/bash
# Get the content of frames.ts to see how the regex patterns are imported and used
cat utils/frames.ts
# Check if there are any test files in the utils directory
fd -e test.ts -e spec.ts . utils/
Length of output: 9380
data/helpers/messages/handleGroupUpdatedMessage.ts (1)
6-6: Verify consistent type import across codebase
The import path change aligns with the PR objective of relocating XMTP client types.
✅ Verification successful
Import path change is correctly implemented
The type DecodedMessageWithCodecsType is properly defined in client.types.ts and all its usages across the codebase consistently import it from @utils/xmtpRN/client.types. This includes:
data/helpers/messages/handleGroupUpdatedMessage.tsdata/helpers/messages/handleGroupUpdatedMessage.test.tsutils/xmtpRN/messages.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of the new import path across the codebase
# Search for old import path
rg "from \"@utils/xmtpRN/client\".*DecodedMessageWithCodecsType"
# Search for new import path usage
rg "from \"@utils/xmtpRN/client.types\".*DecodedMessageWithCodecsType"
Length of output: 135
Script:
#!/bin/bash
# Let's try a broader search to understand the usage of DecodedMessageWithCodecsType
# and verify the import paths
# Search for any occurrence of DecodedMessageWithCodecsType
rg "DecodedMessageWithCodecsType"
# Also check if the type might be defined in client.types.ts
fd -e ts "client.types.ts" --exec cat {}
Length of output: 2301
components/Chat/Frame/FramesPreviews.tsx (2)
23-28: Good use of zustand store with shallow comparison
The switch to zustand with shallow comparison optimizes performance by preventing unnecessary re-renders.
42-47: Verify the component recycling fix
The useEffect hook handles component recycling, but we should verify the cleanup behavior.
✅ Verification successful
Let me gather more information about the cleanup behavior in useEffect.
Let me check the specific component's implementation to understand the cleanup requirements better.
Component recycling is properly handled
The current implementation correctly handles component recycling by using a useRef to track message ID changes and prevent unnecessary re-fetches. No cleanup is required in this case because:
- The
messageIdref is used only for comparison and doesn't need cleanup - The
fetchTagsIfNeededcall is properly memoized (included in deps array) - The component follows the same pattern as other recycling handlers in the codebase (as seen in the search results)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for similar patterns in other components to ensure consistent handling
# Search for similar useEffect patterns with messageId
rg "useEffect.*messageId.*current" -A 3
# Search for other recycling handling patterns
rg "Components are recycled" -A 5
Length of output: 1347
Script:
#!/bin/bash
# Search for cleanup functions in useEffect hooks
ast-grep --pattern 'useEffect(() => {
$$$
return () => {
$$$
}
}, $$$)'
# Also check for any messageId refs cleanup
rg "messageId.*cleanup" -A 3
Length of output: 41387
Script:
#!/bin/bash
# Get the full implementation of FramesPreviews component
ast-grep --pattern 'const FramesPreviews = $$$'
# Also check for any state updates in the component
rg "messageId" components/Chat/Frame/FramesPreviews.tsx -A 2
Length of output: 465
utils/events.ts (1)
3-3: LGTM: Import path updated correctly
The import path change aligns with the PR objective of centralizing XMTP client types.
Let's verify the type relocation:
✅ Verification successful
Import path correctly updated and type definition verified
The GroupWithCodecsType is properly defined in the new location at utils/xmtpRN/client.types.ts, confirming the successful relocation of the type definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that GroupWithCodecsType is properly defined in the new location
ast-grep --pattern 'export type GroupWithCodecsType = $_'
Length of output: 267
utils/xmtpRN/attachments.ts (1)
1-2: LGTM: Import paths updated correctly
The import path changes align with the PR objective of centralizing XMTP client types.
Let's verify both type relocations:
✅ Verification successful
LGTM: Import paths are correctly aligned with type definitions
Both types are properly defined in their new locations:
XmtpMessageis defined indata/store/chatStore.tsConverseXmtpClientTypeis defined inutils/xmtpRN/client.types.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that both types are properly defined in their new locations
echo "Checking XmtpMessage type..."
ast-grep --pattern 'export type XmtpMessage = $_'
echo "Checking ConverseXmtpClientType..."
ast-grep --pattern 'export type ConverseXmtpClientType = $_'
Length of output: 1019
components/Chat/ChatPlaceholder/GroupChatPlaceholder.tsx (1)
6-6: LGTM! Clean implementation of topic-based placeholder logic
The addition of isGroupTopic utility and topic context helps provide more specific feedback to users.
Also applies to: 29-29
config.ts (1)
48-48: Verify security implications of allowed URL schemes
The addition of URL schemes looks good, but we should ensure proper URL validation is implemented when handling these schemes.
✅ Verification successful
Let me gather more specific information about frame URL validation.
Let me search for frame URL validation implementation.
Based on the gathered information, I can now provide the final response:
URL scheme validation is properly implemented with multiple security layers
The codebase has robust URL validation through:
- URL scheme whitelist in config (
framesAllowedSchemes: ["http", "https", "ethereum"]) - Standard URL validation using native
URLconstructor for parsing - URL regex pattern matching via
URL_REGEXthat validates protocol and domain structure - Frame-specific validation through
validateFrame()function that checks accepted clients
The implementation follows security best practices by:
- Explicitly whitelisting allowed schemes
- Using native URL parsing for scheme extraction
- Validating URLs before processing frames
- Normalizing URLs to lowercase to prevent case-sensitivity bypasses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for URL validation logic in the codebase
rg -A 5 "validateUrl|validateUri|isValidUrl|isValidUri"
# Search for URL handling/processing code
ast-grep --pattern 'new URL($$$)'
Length of output: 1319
Script:
#!/bin/bash
# Search for frame-related URL validation
rg -A 10 "frame.*url|url.*frame" -i
# Look for frame URL processing in Frame components
ast-grep --pattern 'function $_($$$) {
$$$
url $$$
$$$
}' components/Chat/Frame/
# Search for URL scheme validation
rg -A 5 "scheme|protocol.*valid|valid.*protocol" -i
Length of output: 71396
Script:
#!/bin/bash
# Look for frame URL validation in frames.ts
rg -A 10 "validateFrame|validate.*url" utils/frames.ts
# Search for URL validation in regex.ts
cat utils/regex.ts
# Look for frame URL processing and validation
ast-grep --pattern 'function $_($$$) {
$$$
if ($$$url$$$) {
$$$
}
$$$
}' utils/frames.ts
Length of output: 2958
data/helpers/messages/handleGroupUpdatedMessage.test.ts (1)
4-4: LGTM! Import change aligns with type refactoring.
The updated import path for DecodedMessageWithCodecsType from @utils/xmtpRN/client.types is consistent with the PR's objective of centralizing type definitions.
Let's verify the consistency of this type import across the codebase:
✅ Verification successful
Import change is correctly implemented and consistent
The verification confirms that:
- The type
DecodedMessageWithCodecsTypeis properly defined inutils/xmtpRN/client.types.ts - All imports of this type are consistently using the new path
@utils/xmtpRN/client.types - No legacy imports remain in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports of DecodedMessageWithCodecsType from the old location
rg "DecodedMessageWithCodecsType.*from.*@utils/xmtpRN/client[^.]" -l
Length of output: 70
Script:
#!/bin/bash
# Let's check all imports of DecodedMessageWithCodecsType to ensure consistency
rg "DecodedMessageWithCodecsType" -l
# Also check the actual type definition location
fd "client.types.ts"
Length of output: 247
Script:
#!/bin/bash
# Let's check the actual imports in these files to verify consistency
rg "import.*DecodedMessageWithCodecsType" -A 1
# Also check the type definition to ensure it exists in client.types.ts
cat utils/xmtpRN/client.types.ts | rg "type.*DecodedMessageWithCodecsType"
Length of output: 605
data/db/index.ts (1)
88-88: LGTM! Improved logging consistency.
Changed console.log to logger.debug for better logging consistency and observability.
utils/logout/index.tsx (1)
3-4: LGTM! Import restructuring aligns with type centralization.
The separation of the type import from the implementation file improves code organization.
Let's verify the consistency of this type import across the codebase:
✅ Verification successful
Import restructuring is correctly implemented across the codebase
All imports of ConverseXmtpClientType are consistently sourcing from @utils/xmtpRN/client.types, and there are no remaining imports from the old location. The type centralization is properly maintained throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining imports of ConverseXmtpClientType from the old location
rg "ConverseXmtpClientType.*from.*@utils/xmtpRN/client[^.]" -l
Length of output: 64
Script:
#!/bin/bash
# Let's try a broader search to find all imports of ConverseXmtpClientType
rg "ConverseXmtpClientType" -A 1
# Also search for any imports from client.types specifically
rg "from.*@utils/xmtpRN/client\.types" -A 1
Length of output: 11276
utils/xmtpRN/sync.ts (1)
3-3: LGTM: Type import relocation
The change aligns with the PR's objective of relocating XMTP client types to a dedicated types file.
utils/frames.ts (3)
124-124: LGTM: Improved frame storage with message ID association
The change enhances type safety by explicitly associating frames with message IDs in the store.
273-286: LGTM: Enhanced frame message validation
The refactored function improves clarity by:
- Separating message type and content validation
- Adding proper content sanitization
- Making the frame store dependency explicit
288-294: LGTM: Added utility for frame existence check
New utility function provides a clear and type-safe way to check for frame existence by message ID.
screens/Conversation.tsx (2)
118-119: LGTM: Fixed potential race condition
The additional check for conversationTopicRef.current prevents unnecessary main conversation opening when a topic is already set.
Line range hint 290-305: LGTM: Enhanced conversation context with topic
Adding topic to the conversation context improves:
- Type safety through explicit topic propagation
- Component reusability by making the topic accessible via context
i18n/translations/en.ts (1)
212-219: LGTM! Well-structured error messages and user guidance.
The new translations provide clear and user-friendly messages for identity-related scenarios, including:
- Identity not found errors
- Resolution timeout cases
- Guidance for non-XMTP users
components/Chat/Frame/FramePreview.tsx (2)
158-162: Good addition of ethereum: protocol support.
The special handling for ethereum: protocol links is necessary as they don't follow the standard URL format with double slashes.
354-354: Fixed aspect ratio format.
Corrected the aspect ratio format from "1.91.1" to "1.91:1", which is the correct notation for aspect ratios.
components/Chat/Message/MessageActions.tsx (2)
68-68: LGTM! Good type safety improvement.
The addition of the isFrame prop improves type safety and makes the frame handling more explicit.
Also applies to: 82-82
289-297: LGTM! Proper frame URL handling.
The frame URL handling is well implemented using the frames store and proper memoization.
utils/conversation.ts (2)
223-231: Enhanced error logging implementation
Good implementation of error handling with proper context logging.
The error handling is well-structured with:
- Try-catch block for address resolution
- Detailed error logging with context
- Graceful fallback when address resolution fails
263-264: Well-implemented race condition prevention
The isDone flag effectively prevents multiple alerts and race conditions.
The implementation:
- Prevents multiple alerts from being shown
- Handles race conditions gracefully
- Maintains consistent state management
Also applies to: 270-271, 283-283, 290-290
components/Chat/Chat.tsx (1)
386-388: Well-structured frame state management
Good implementation of frame state management using useFramesStore.
The implementation:
- Uses proper state selection with useSelect
- Maintains consistent state access patterns
- Follows React best practices for state management
Also applies to: 544-546
components/Chat/Message/Message.tsx (2)
170-178: Clean implementation of frame message detection
Good use of the isFrameMessage utility with proper type checking.
The implementation:
- Uses proper type guards
- Maintains clean separation of concerns
- Follows functional programming principles
341-370: Well-structured swipe action implementation
The renderLeftActions implementation is clean and follows React Native best practices.
Notable points:
- Clean animation handling
- Proper use of interpolation
- Good separation of concerns
| @@ -4,6 +4,7 @@ import { awaitableAlert } from "@utils/alert"; | |||
| import { getDbEncryptionKey } from "@utils/keychain/helpers"; | |||
| import logger from "@utils/logger"; | |||
| import { useLogoutFromConverse } from "@utils/logout"; | |||
| import { XmtpClientByAccount } from "@utils/xmtpRN/client.types"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Type changes need to be propagated across the codebase
The verification reveals that while XmtpClientByAccount is properly defined and imported, there are numerous usages of ConverseXmtpClientType across multiple files that should be reviewed for consistency with the new type system:
queries/useGroupQuery.tsqueries/useGroupsQuery.tsutils/notifications.tsutils/logout/index.tsxutils/evm/xmtp.tsutils/xmtpRN/sync.tsutils/xmtpRN/send.tsutils/xmtpRN/attachments.tsutils/xmtpRN/conversations.tsutils/xmtpRN/messages.tsscreens/Profile.tsx
🔗 Analysis chain
LGTM! Type improvements enhance code maintainability
The migration to a dedicated type from client.types.ts improves code organization and type safety.
Also applies to: 57-57
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of the new type across the codebase
rg -A 2 "XmtpClientByAccount"
# Check for any remaining old type references
rg "ConverseXmtpClientType"
Length of output: 5418
TL;DR
Refactored XMTP client types and improved logging for group refresh operations.
What changed?
client.types.ts)refreshGroupfunction.aider*to.gitignoreResulttype for better error handlingconsole.logwithlogger.debugininitDbfunctionWhy make this change?
Summary by CodeRabbit
New Features
Bug Fixes
getUrlToRenderfunction to prevent failures on invalid URLs.Documentation
README.mdwith a new section on release processes and rebasing branches.Chores
Style
Tests
Refactor
Conversationcomponent.