fix: Fixed type to match new SDK Version #1404
Conversation
Added senderInboxId
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
Hmm no I don't think so, I'll try to find a log for it, likely related to conversation context not getting set correctly |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
features/conversation/hooks/use-send-message.ts (1)
82-84: Consider addressing the @ts-ignore comment.The presence of @ts-ignore for the deliveryStatus property suggests potential type mismatches. Consider properly typing IConversationMessageStatus in the SDK types or creating a proper type intersection.
Example approach:
type OptimisticDecodedMessage<T> = DecodedMessage<T> & { deliveryStatus: IConversationMessageStatus; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/conversation/hooks/use-send-message.ts(1 hunks)
🔇 Additional comments (1)
features/conversation/hooks/use-send-message.ts (1)
85-85: LGTM! Verify complete SDK type implementation.
The change from senderAddress to senderInboxId aligns with the SDK update. The value is correctly sourced from currentUserInboxId.
Let's verify if there are any other instances where sender identification needs to be updated:
✅ Verification successful
The change to senderInboxId is part of a broader SDK update and is correctly implemented
The codebase search reveals that senderInboxId is consistently used across the application, particularly in message-related functionality. The change from senderAddress to senderInboxId in the use-send-message.ts file aligns with this pattern. The few remaining instances of senderAddress are either:
- In comments explaining legacy behavior
- In type definitions for specific data structures
- In iOS notification extension code that handles address translation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential missed updates to sender identification
rg -i "senderAddress|senderInbox" --type typescript
Length of output: 88
Script:
#!/bin/bash
# Search for potential missed updates to sender identification using ripgrep without file type filter
rg -i "senderAddress|senderInbox"
Length of output: 10805
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.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
Line range hint
315-317: Fix type inconsistencies in test fixtures.There are several type-related issues in the test fixtures that should be addressed:
- Fix the incorrect variable reference:
- return fixtureGroupsDataEntity; + return fixtureConversationDataEntity;
- Align the return types:
- ): Promise<GroupsDataEntity> => { + ): Promise<ConversationDataEntity> => {
- Remove @ts-expect-error comments by properly typing the fixtures:
- // @ts-expect-error - return fixtureGroupsDataEntity; + return { + ids: [GroupIdUserIsAlreadyAMemberOf], + byId: { + [GroupIdUserIsAlreadyAMemberOf]: conversationFixture, + }, + } as ConversationDataEntity;Also applies to: 386-388, 469-471
🧹 Nitpick comments (1)
features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
Line range hint
132-196: Consider enhancing the polling mechanism.The implementation is solid with good error handling and logging. However, consider these improvements:
- Implement exponential backoff instead of fixed intervals to reduce server load
- Make polling parameters configurable via options
- Add an overall timeout parameter separate from attempt count
Example implementation with exponential backoff:
- const GROUP_JOIN_REQUEST_POLL_INTERVAL_MS = 1000; + const BASE_POLL_INTERVAL_MS = 1000; + const MAX_POLL_INTERVAL_MS = 5000; while (attemptsToRetryJoinGroup < GROUP_JOIN_REQUEST_POLL_MAX_ATTEMPTS) { // ... existing code ... - await wait(GROUP_JOIN_REQUEST_POLL_INTERVAL_MS); + const backoffInterval = Math.min( + BASE_POLL_INTERVAL_MS * Math.pow(2, attemptsToRetryJoinGroup), + MAX_POLL_INTERVAL_MS + ); + await wait(backoffInterval); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
features/GroupInvites/joinGroup/JoinGroup.client.ts(1 hunks)
🔇 Additional comments (1)
features/GroupInvites/joinGroup/JoinGroup.client.ts (1)
27-30: LGTM! Well-defined polling parameters.
The polling configuration with 10 attempts at 1-second intervals provides a reasonable balance between responsiveness and server load.
should have been fixed in previous PR. You've seen it again? |
Added senderInboxId
Summary by CodeRabbit