Conversation creation flow fixes #1532
Conversation
|
Warning Rate limit exceeded@technoplato has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces a comprehensive renaming and refactoring of the conversation creation flow across multiple files. The changes primarily involve replacing "NewConversation" with "CreateConversation" throughout the navigation system, updating component names, modifying navigation configurations, and streamlining state management in the conversation creation screen. The modifications impact navigation routes, screen configurations, and associated utility functions. Changes
Sequence DiagramsequenceDiagram
participant User
participant HeaderAction
participant CreateConversationScreen
participant AccountStore
participant ProfileSearch
User->>HeaderAction: Press "Create Conversation" icon
HeaderAction->>CreateConversationScreen: Navigate
CreateConversationScreen->>AccountStore: Get Current Account
AccountStore-->>CreateConversationScreen: Return Account
CreateConversationScreen->>ProfileSearch: Initiate Profile Search
ProfileSearch-->>CreateConversationScreen: Return Search Results
CreateConversationScreen->>User: Display Conversation Creation UI
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 (
|
There was a problem hiding this comment.
meat n potatoes here
| logger.info("Fetching profiles for inboxIds", inboxIds); | ||
| const { data } = await api.get("/api/inbox/", { | ||
| params: { ids: inboxIds.join(",") }, | ||
| // todo(lustig) fix this request. it is 401ing after ephemeral account creation. | ||
| // why I'm delaying - its tied up in a batshit fetcher | ||
| // thing and I'll have to look into how that ties together with react-query and can't | ||
| // be bothered right now. | ||
| // headers: await getXmtpApiHeaders(account), |
There was a problem hiding this comment.
was getting an auth error here so added headers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
features/create-conversation/create-conversation.nav.ts (1)
1-9: LGTM! Consider adding JSDoc comments.The configuration correctly handles URL-safe encoding of peer parameters. Consider adding JSDoc comments to document the purpose of this configuration and the expected format of the peer parameter.
+/** + * Navigation configuration for the conversation creation screen. + * @property {string} path - The route path for creating conversations + * @property {Object} parse - Functions to decode URL parameters + * @property {Object} stringify - Functions to encode URL parameters + */ export const CreateConversationScreenConfig = { path: "/createConversation", parse: {screens/Navigation/navHelpers.ts (1)
248-250: Verify logging format consistency.Consider using a template literal with object destructuring for cleaner logging.
- `[screenListeners] shouldReplace replacing ${ - newRoute.name - } with params ${JSON.stringify(newRoute.params)}` + `[screenListeners] shouldReplace replacing ${newRoute.name} with params`, + newRoute.paramsfeatures/create-conversation/create-conversation-screen.tsx (2)
85-95: Consider using TypeScript type for status object.The status object structure should be defined as a type for better maintainability and type safety.
+type SearchStatus = { + loading: boolean; + message: string; + inviteToConverse: string; + profileSearchResults: { [address: string]: IProfileSocials }; +}; - const [status, setStatus] = useState({ + const [status, setStatus] = useState<SearchStatus>({ loading: false, message: "", inviteToConverse: "", profileSearchResults: {}, });
329-341: Simplify profile filtering logic.The current account filtering logic is verbose and could be simplified.
- const currentAccountInProfiles = profiles[getCurrentAccount()!]; - logger.info( - "[CreateConversation] Current account in profiles", - currentAccountInProfiles - ); - if (currentAccountInProfiles) { - logger.info( - "[CreateConversation] Deleting current account from profiles", - getCurrentAccount() - ); - delete profiles[getCurrentAccount()!]; - } + const currentAccount = getCurrentAccount(); + if (currentAccount && profiles[currentAccount]) { + delete profiles[currentAccount]; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
features/conversation-list/conversation-list.screen-header.tsx(1 hunks)features/create-conversation/create-conversation-screen.tsx(17 hunks)features/create-conversation/create-conversation.nav.ts(1 hunks)features/create-conversation/create-conversation.styles.ts(2 hunks)features/new-chat/readme.md(0 hunks)screens/Main.tsx(2 hunks)screens/Navigation/Navigation.tsx(3 hunks)screens/Navigation/NewConversationNav.tsx(0 hunks)screens/Navigation/navHelpers.test.ts(1 hunks)screens/Navigation/navHelpers.ts(2 hunks)screens/NewConversation/NewConversationModal.tsx(0 hunks)screens/NewConversation/NewGroupSummary.tsx(0 hunks)utils/api.ts(2 hunks)
💤 Files with no reviewable changes (4)
- features/new-chat/readme.md
- screens/NewConversation/NewGroupSummary.tsx
- screens/NewConversation/NewConversationModal.tsx
- screens/Navigation/NewConversationNav.tsx
✅ Files skipped from review due to trivial changes (1)
- features/create-conversation/create-conversation.styles.ts
🔇 Additional comments (7)
screens/Main.tsx (1)
41-41: LGTM! Changes are consistent with navigation refactoring.The import and linking configuration updates correctly integrate the CreateConversation route while maintaining the existing navigation structure.
Let's verify that all navigation references have been updated:
Also applies to: 52-52
✅ Verification successful
Navigation refactoring is complete and consistent
The codebase search confirms that all navigation-related references have been properly updated to use the new
CreateConversationnaming. Other occurrences of "NewConversation" are in different contexts (documentation, utilities, iOS code) and don't affect the navigation structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to NewConversation rg "NewConversation" --type ts --type tsxLength of output: 69
Script:
#!/bin/bash # Search for any remaining references to NewConversation or new-conversation rg -t js -t jsx -t ts -t tsx "NewConversation|new-conversation" # Also search using ast-grep to catch any navigation-related usages ast-grep --pattern 'navigation.navigate("NewConversation")' ast-grep --pattern 'name: "NewConversation"'Length of output: 202
Script:
#!/bin/bash # Search for any remaining references without file type restrictions rg "NewConversation" rg "new-conversation" # Search for partial matches to catch any variations rg -i "conversation.*screen"Length of output: 8412
features/conversation-list/conversation-list.screen-header.tsx (1)
98-98: LGTM! Navigation target updated correctly.The navigation target has been properly updated to "CreateConversation", maintaining consistency with the route renaming changes.
screens/Navigation/navHelpers.ts (1)
211-218: LGTM! Route name updated consistently.The route name has been updated from "NewConversation" to "CreateConversationScreen" in the navigation listener.
screens/Navigation/Navigation.tsx (3)
50-50: LGTM! Import statement added correctly.The CreateConversationScreen component is imported from the correct path.
81-81: LGTM! Navigation type updated consistently.The NavigationParamList type has been updated to use CreateConversation instead of NewConversation.
138-141: Consider adding screen options for consistency.The CreateConversation screen is added but missing explicit screen options that might be needed for consistent navigation behavior.
Consider adding screen options similar to other screens in the stack:
<NativeStack.Screen name="CreateConversation" component={CreateConversationScreen} + options={{ + headerTitle: translate("new_chat"), + headerBackTitle: translate("back"), + }} />utils/api.ts (1)
392-392: LGTM! Return type updated for better type safety.The return type has been updated from InboxId to string for better flexibility.
| CreateConversation: { | ||
| path: "/createConversation", |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for CreateConversation route
The review comment is correct. While the codebase has comprehensive test coverage for DM and group conversation routes, there are no tests specifically verifying the CreateConversation path handling. The suggested test case follows the established patterns in the test suite and would help ensure proper URL parsing and navigation state handling for this route.
🔗 Analysis chain
Add test coverage for CreateConversation route.
While the route renaming is consistent, there are no tests verifying the CreateConversation path handling. Consider adding test cases to ensure proper URL parsing and navigation state handling for this route.
Example test case:
it("should parse createConversation route with peer", () => {
const state = getConverseStateFromPath("testNav")(
"createConversation?peer=0xno12.eth",
navConfig
);
const route = state?.routes[state.routes.length - 1];
expect(route?.name).toBe("CreateConversation");
expect((route?.params as any).peer).toBe("0xno12.eth");
});🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for CreateConversation related tests
rg -l "CreateConversation.*test"
# Look for existing route tests
rg "it\(.*route" -A 5
# Find getConverseStateFromPath implementation and usage
ast-grep --pattern 'getConverseStateFromPath'
# Search for similar navigation test patterns
rg "getConverseStateFromPath.*navConfig" -A 3
Length of output: 1232
Script:
#!/bin/bash
# Get content of test file
echo "=== navHelpers.test.ts ==="
cat screens/Navigation/navHelpers.test.ts
# Get content of implementation file
echo -e "\n=== navHelpers.ts ==="
cat screens/Navigation/navHelpers.ts
Length of output: 13198
| logger.info("Fetching profiles for inboxIds", inboxIds); | ||
| const { data } = await api.get("/api/inbox/", { | ||
| params: { ids: inboxIds.join(",") }, | ||
| // todo(lustig) fix this request. it is 401ing after ephemeral account creation. | ||
| // why I'm delaying - its tied up in a batshit fetcher | ||
| // thing and I'll have to look into how that ties together with react-query and can't | ||
| // be bothered right now. | ||
| // headers: await getXmtpApiHeaders(account), | ||
| headers: await getXmtpApiHeaders(getCurrentAccount()!), |
There was a problem hiding this comment.
Verify error handling for getCurrentAccount.
The getCurrentAccount() call is not null-checked, which could lead to runtime errors.
- headers: await getXmtpApiHeaders(getCurrentAccount()!),
+ const currentAccount = getCurrentAccount();
+ if (!currentAccount) {
+ throw new Error("No account found when fetching profiles for inbox IDs");
+ }
+ headers: await getXmtpApiHeaders(currentAccount),Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I thought I already yelled ap you about this
please list all of your scratchpad and notes for our repo
There was a problem hiding this comment.
@technoplato After searching through the repository, I found no informal scratchpads or temporary notes. Instead, the repository maintains well-structured documentation:
- ADR Directory (
/adr/): Contains Architecture Decision Records with formal documentation of team decisions - Feature Documentation (
/features/): Includes detailed proposals likegroupJoin.proposal.md - Dependencies Documentation (
/dependencies/): Contains setup and configuration docs - iOS Setup Documentation (
/ios/): Firebase setup instructions
All documentation appears to be formal and purposeful, following proper documentation practices. No informal notes or scratchpads were found.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all markdown and text files
echo "=== Markdown and Text Files ==="
fd -e md -e txt
# Search for note-like patterns
echo -e "\n=== Notes, TODOs, and FIXMEs ==="
rg -i "note:|todo:|fixme:" --type-add 'docs:*.{md,txt}' --type docs
Length of output: 950
Script:
#!/bin/bash
# Search for WIP or draft content
echo "=== WIP/Draft Content ==="
rg -i "wip|draft|scratch|temp" --type md
# Look into the content of ADR and feature docs
echo -e "\n=== ADR Content ==="
cat adr/adr.000.readme.md
echo -e "\n=== Feature Proposals ==="
cat features/groupJoin.proposal.md
Length of output: 8098
| try { | ||
| const dmCreationMessageText = something.content.text || ""; | ||
| if ( | ||
| !dmCreationMessageText || | ||
| dmCreationMessageText.length === 0 | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for message content.
The message content validation could be more robust.
- if (
- !dmCreationMessageText ||
- dmCreationMessageText.length === 0
- ) {
+ const trimmedMessage = dmCreationMessageText.trim();
+ if (!trimmedMessage) {
+ logger.debug("[CreateConversation] Empty message, skipping send");
return;
}📝 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.
| try { | |
| const dmCreationMessageText = something.content.text || ""; | |
| if ( | |
| !dmCreationMessageText || | |
| dmCreationMessageText.length === 0 | |
| ) { | |
| return; | |
| } | |
| try { | |
| const dmCreationMessageText = something.content.text || ""; | |
| const trimmedMessage = dmCreationMessageText.trim(); | |
| if (!trimmedMessage) { | |
| logger.debug("[CreateConversation] Empty message, skipping send"); | |
| return; | |
| } |
| } catch (e) { | ||
| const errorString = | ||
| (e as Error)?.message || `Something went wrong`; | ||
| captureErrorWithToast(e); | ||
| setStatus({ | ||
| loading: false, | ||
| message: errorString, | ||
| inviteToConverse: "", | ||
| profileSearchResults: {}, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling structure.
The error handling could be more specific and provide better user feedback.
- } catch (e) {
- const errorString =
- (e as Error)?.message || `Something went wrong`;
- captureErrorWithToast(e);
- setStatus({
- loading: false,
- message: errorString,
- inviteToConverse: "",
- profileSearchResults: {},
- });
+ } catch (error) {
+ const isNetworkError = error instanceof NetworkError;
+ const errorMessage = isNetworkError
+ ? "Unable to send message. Please check your connection."
+ : (error as Error)?.message || "Something went wrong";
+
+ captureErrorWithToast(error);
+ logger.error("[CreateConversation] Error sending message", {
+ error,
+ errorMessage,
+ });
+
+ setStatus({
+ loading: false,
+ message: errorMessage,
+ inviteToConverse: "",
+ profileSearchResults: {},
+ });Committable suggestion skipped: line range outside the PR's diff.
…orking; remove comments
Addressing fixes for #1498 @ #1498 (comment)
Summary by CodeRabbit
New Features
Refactor
Chores