Ml/fix deeplink to groupchat #1395
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a new utility function Changes
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
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 (
|
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: 1
🧹 Nitpick comments (3)
utils/navigation.ts (2)
13-25: Validate ref before setting.
The function checks if the ref is already set or null. Good defensive programming. However, consider whether you want to replace an existing ref if something triggers re-mounting. If replacing the ref is never valid, this is fine as is.
49-51: Refine TypeScript ignore.
Instead of ignoring TypeScript, define a more specific type for the generic navigate method if possible. This ensures type safety around screen names and their params.- // @ts-ignore - converseNavigatorRef.navigate(screen, params); + // TODO: Provide a more specific type for the navigate method + converseNavigatorRef.navigate( + screen as keyof NavigationParamList, + params as NavigationParamList[keyof NavigationParamList] + );data/store/appStore.ts (1)
90-114: Polling loop for hydration.
This loop polls every 100ms until hydration is complete.
• Consider making the delay configurable.
• Ensure there is a maximum timeout to avoid potential infinite loops.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
data/store/appStore.ts(2 hunks)features/notifications/utils/onInteractWithNotification.ts(2 hunks)screens/Main.tsx(4 hunks)utils/navigation.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: tsc
screens/Main.tsx
[failure] 25-25:
Module '"../utils/navigation"' has no exported member 'converseNavigations'.
[failure] 26-26:
'"../utils/navigation"' has no exported member named 'converseNavigatorRef'. Did you mean 'setConverseNavigatorRef'?
🔇 Additional comments (10)
utils/navigation.ts (3)
8-8: Ensure correct import usage.
Importing the NavigationContainerRef looks correct and aligns well with the rest of the file. No action required.
10-11: Initialize reference to null.
This explicit null initialization is clear and helps avoid referencing an uninitialized ref. No further changes needed.
31-41: Check readiness carefully.
Confirm that any calling code or user interactions don't happen before the navigator is ready. The early returns are good.
features/notifications/utils/onInteractWithNotification.ts (4)
2-2: Consolidate imports.
Imports look consistent, and the new references from "@utils/navigation" are used correctly. No issues found.
7-7: Hydration function usage.
The explicit import of waitForXmtpClientHydration clarifies dependency on app store readiness. This is a clean approach.
13-13: Logging for debugging.
Logging upon interaction with a notification is helpful for troubleshooting. Good practice.
58-58: Awaiting store hydration.
Calling waitForXmtpClientHydration ensures the client is fully ready, preventing race conditions. Well done.
data/store/appStore.ts (1)
5-5: Logger import looks good.
No issues found with importing the logger utility.
screens/Main.tsx (2)
92-95: Logging the ref state.
Logging whether the ref is null helps with debugging. Looks good.
114-114: Splash screen gating.
Properly waiting for splash screen logic to complete before rendering navigation is a neat pattern.
data/store/appStore.ts
Outdated
| logger.debug( | ||
| `[waitForAppStoreHydration] App store hydration not complete, waiting 100ms...` | ||
| ); | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
Why are you doing this vs creating a subscription to the store instead?
There was a problem hiding this comment.
Yes I thought about that too. But It would look something like that no? Which isn't cleaner IMO or even much more performant. Maybe we keep what Michael have for now which is good enough IMO.
And we'll probably have to refactor stuff anyway later
const waitForReady = (): Promise<void> => {
return new Promise((resolve) => {
const { isReady } = useAccountsStore.getState();
if (isReady) {
resolve();
return;
}
const unsubscribe = useAccountsStore.subscribe(
(state) => state.isReady,
(ready) => {
if (ready) {
resolve();
unsubscribe(); // Stop listening after `isReady` becomes true
}
}
);
});
};
There was a problem hiding this comment.
you guys are right I like this better. It would also resolve sooner (rather than waiting for the 100ms window to elapse)
There was a problem hiding this comment.
here's a loom with this change: https://www.loom.com/share/d1967df857fb4d1984cbdce195ff7739 (explaining the reason for the wait(1)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
dependencies/Environment/Flavors/determineFlavor.utils.ts (1)
4-8: Temporary Jest check needs clear removal criteriaThe TODO comment should specify when and under what conditions this Jest environment check should be removed. Additionally, using
@ts-ignorebypasses type safety - consider using proper type guards instead.Consider this alternative implementation:
- // todo(lustig): remove this once we have a better way to determine the flavor - // @ts-ignore - if (typeof jest !== "undefined" || process.env.JEST_WORKER_ID !== undefined) { + // TODO(lustig): Remove Jest check after implementing proper test environment detection (TICKET-123) + if ( + (typeof jest !== "undefined" && jest !== null) || + typeof process !== "undefined" && process.env.JEST_WORKER_ID !== undefined + ) {data/store/appStore.ts (1)
105-130: Consider memory leak and error handling improvementsWhile the implementation is solid, there are two areas for improvement:
- The subscription should be cleaned up if the timeout occurs first
- The error message could include more diagnostic information
Consider this enhanced implementation:
export const waitForXmtpClientHydration = ( timeout: number = 1000 ): Promise<void> => { + let unsubscribe: (() => void) | undefined; + const hydrationPromise = new Promise<void>((resolve) => { const { hydrationDone } = useAppStore.getState(); if (hydrationDone) { resolve(); return; } - const unsubscribe = useAppStore.subscribe((state, prevState) => { + unsubscribe = useAppStore.subscribe((state, prevState) => { if (state.hydrationDone && !prevState.hydrationDone) { resolve(); unsubscribe(); } }); }); const timeoutPromise = new Promise<void>((_, reject) => { setTimeout(() => { - reject(new Error("Xmtp client hydration timed out")); + if (unsubscribe) { + unsubscribe(); // Clean up subscription on timeout + } + const { hydrationDone } = useAppStore.getState(); + reject(new Error( + `Xmtp client hydration timed out after ${timeout}ms. Current hydration state: ${hydrationDone}` + )); }, timeout); }); return Promise.race([hydrationPromise, timeoutPromise]); };screens/Main.tsx (1)
88-91: Enhance navigation ref loggingWhile logging the navigation ref state is helpful, consider including more context for better debugging.
- logger.info( - `[Main] Setting navigation ref to ${r ? "not null" : "null"}` - ); + logger.info( + `[Main] Setting navigation ref to ${r ? "not null" : "null"}`, + { + currentScreen: r?.getCurrentRoute()?.name, + timestamp: new Date().toISOString(), + } + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
data/store/appStore.ts(2 hunks)dependencies/Environment/Flavors/determineFlavor.utils.ts(1 hunks)screens/Main.tsx(4 hunks)
🔇 Additional comments (2)
screens/Main.tsx (2)
110-110: LGTM: Simplified state selection
Good job simplifying the state selection to only include the required splashScreenHidden property.
24-24: Verify consistent navigation ref usage
Let's ensure the navigation ref is consistently used across the codebase.
✅ Verification successful
Navigation ref implementation is consistent and well-structured
The navigation ref implementation follows a consistent pattern across the codebase:
- Central navigation utility in
utils/navigation.tsmanages theconverseNavigatorRef - Main screen correctly sets up the navigation ref using
setConverseNavigatorRef - Navigation references in other components (like
ConversationFlashList) are properly scoped - Navigation structure is well-organized under dedicated navigation components in the
screens/Navigationdirectory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of navigation utilities
# Search for direct NavigationContainer ref assignments
echo "Checking for direct ref assignments..."
rg -l "ref={" --type ts --type tsx
# Search for navigation ref usage
echo "Checking navigation ref usage..."
rg "converseNavigatorRef|setConverseNavigatorRef" --type ts --type tsx
# Search for potential navigation-related TODOs
echo "Checking navigation-related TODOs..."
rg "TODO.*navig" --type ts --type tsx
Length of output: 524
Script:
#!/bin/bash
# Let's fix the syntax and search patterns
# Search for navigation ref assignments and usage
echo "Checking navigation patterns..."
rg -i "navigationref|navigatorref" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts'
# Search for navigation utility imports and definitions
echo "Checking navigation utilities..."
rg "setConverseNavigatorRef|converseNavigatorRef" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts'
# Find all navigation-related files
echo "Finding navigation-related files..."
fd -e ts -e tsx | rg -i "navigat"
Length of output: 2707
…it instead of sleep
| // note(lustig): this is a hack to ensure that the next render has completed | ||
| // as this is used to navigate to a conversation currently. | ||
| // We'll revisit this and make something that doesn't suck as much later. | ||
| await wait(1); |
There was a problem hiding this comment.
explained here: https://www.loom.com/share/d1967df857fb4d1984cbdce195ff7739
| @@ -24,11 +24,11 @@ import { JoinGroupResult } from "./joinGroup.types"; | |||
| import { ConversationListQueryData } from "@/queries/useConversationListQuery"; | |||
| import { entify } from "@/queries/entify"; | |||
| import { GroupWithCodecsType } from "@/utils/xmtpRN/client"; | |||
| import { wait } from "@/utils/wait"; | |||
There was a problem hiding this comment.
this change is because I realized we had a utility for this already
| @@ -58,40 +55,13 @@ export const onInteractWithNotification = async ( | |||
| | undefined; | |||
|
|
|||
| if (conversationTopic) { | |||
| // todo(lustig): zod verification of external payloads such as those from | |||
| // notifications, deep links, etc | |||
| await waitForXmtpClientHydration(); | |||
There was a problem hiding this comment.
Turns out all the crap I was doing before in the previous diff uh was done in client hydration so it's not needed here
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
524-526: Consider using a shorter timeout in test fixtures.The 5-second wait in the test fixture might slow down the test suite unnecessarily.
Consider reducing the timeout:
- await wait(5000); + await wait(1000); // 1 second should be sufficient for testing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
data/store/appStore.ts(2 hunks)features/GroupInvites/joinGroup/JoinGroup.client.ts(3 hunks)
🔇 Additional comments (3)
data/store/appStore.ts (2)
91-103: LGTM! Well-documented hydration requirements.
The documentation clearly outlines the requirements and current state of XMTP client hydration, which will be valuable for future maintenance and refactoring.
127-131: 🛠️ Refactor suggestion
Consider implementing a more robust solution for render completion.
The current implementation uses a 1ms wait as a temporary solution to ensure render completion. While functional, this approach is fragile and depends on timing assumptions.
Consider implementing one of these alternatives:
- Use React's
useEffectcleanup orflushSync - Implement a proper render completion callback
- Use a state-based approach to track render completion
- // Wait for the next render to complete
- // note(lustig): this is a hack to ensure that the next render has completed
- // as this is used to navigate to a conversation currently.
- // We'll revisit this and make something that doesn't suck as much later.
- await wait(1);
+ // Option 1: Use React's useEffect cleanup
+ await new Promise<void>((resolve) => {
+ const cleanup = useEffect(() => {
+ resolve();
+ return cleanup;
+ }, []);
+ });Likely invalid or redundant comment.
features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
189-191: LGTM! Consistent use of wait utility.
Good replacement of custom sleep implementation with the shared wait utility, improving code consistency.
| logger.info( | ||
| `[Main] Setting navigation ref to ${r ? "not null" : "null"}` | ||
| ); | ||
| setConverseNavigatorRef(r); |
There was a problem hiding this comment.
We only have one navigation ref now that we are no longer supporting different layouts and stuff so I did some refactoring to reflect that
| const { splashScreenHidden } = useAppStore( | ||
| useSelect(["notificationsPermissionStatus", "splashScreenHidden"]) | ||
| ); | ||
| const { splashScreenHidden } = useAppStore(useSelect(["splashScreenHidden"])); |
https://www.loom.com/share/d1967df857fb4d1984cbdce195ff7739
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores