improvement: private message #6734
Conversation
WalkthroughThis pull request introduces a private messages feature for a chat application. The changes include a new test definition for private message workflows, UI components for displaying and dismissing private messages, database schema extensions with a new "private" field (schema version 28), a utility function to delete private messages, updated type definitions, and localization strings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring attention:
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
| comment, | ||
| pinned | ||
| pinned, | ||
| private: isPrivate |
There was a problem hiding this comment.
we can't use private as variable name in class...
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Nice improvement.
I'd like to have e2e tests for the review.
I think sending an unsupported app message (/unsupported) makes you receive a private message like that, so we don't have to configure an app on mobile ws specifically.
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
I am using |
|
I have added E2E testing :) |
…ocket.Chat.ReactNative into app-private-message
Rohit3523
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Rohit3523
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
official_android_build
GitHub Actions
Waiting
Rohit3523
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/lib/methods/deletePrivateMessages.ts (1)
6-26: Consider adding early return and validation for better efficiency.The function could be optimized with these refinements:
- Return early if no messages are found
- Validate the id parameter when provided
- Consider returning a boolean or count to indicate success
export async function deletePrivateMessages(id?: string): Promise<void> { try { const db = database.active; const messages = id ? await db.get('messages').query(Q.where('id', id)).fetch() : await db.get('messages').query(Q.where('private', true)).fetch(); + + if (messages.length === 0) { + return; // No messages to delete + } + const messagesToBeDeleted = messages.map(message => message.prepareDestroyPermanently()); await db.write(async () => { await db.batch(...messagesToBeDeleted); }); } catch (e) { log(e); } }.maestro/tests/assorted/private-message.yaml (1)
22-51: Consider adding assertion for the private indicator text.The test verifies the dismiss button appears and functions correctly. Consider also asserting that the "Only you can see this message" text appears before dismissal to ensure the complete private message UI is rendered.
- extendedWaitUntil: visible: text: 'Status message changed successfully.' timeout: 60000 +- extendedWaitUntil: + visible: + text: 'Only you can see this message' + timeout: 5000 - extendedWaitUntil: visible: text: 'Dismiss message' timeout: 15000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.maestro/tests/assorted/private-message.yaml(1 hunks)app/containers/message/Message.tsx(3 hunks)app/containers/message/index.tsx(2 hunks)app/containers/message/interfaces.ts(1 hunks)app/containers/message/styles.ts(1 hunks)app/definitions/IMessage.ts(1 hunks)app/i18n/locales/en.json(2 hunks)app/lib/database/model/Message.js(2 hunks)app/lib/database/model/migrations.js(1 hunks)app/lib/database/schema/app.js(2 hunks)app/lib/methods/deletePrivateMessages.ts(1 hunks)app/views/RoomView/index.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/views/RoomView/index.tsx (1)
app/lib/methods/deletePrivateMessages.ts (1)
deletePrivateMessages(6-26)
app/containers/message/Message.tsx (1)
app/lib/methods/deletePrivateMessages.ts (1)
deletePrivateMessages(6-26)
🔇 Additional comments (16)
app/containers/message/styles.ts (1)
166-177: LGTM!The new style definitions for private message indicators are straightforward and well-structured.
app/definitions/IMessage.ts (1)
177-177: LGTM!The addition of the optional
privatefield to theIMessageinterface is consistent with the interface structure and properly typed.app/containers/message/index.tsx (1)
413-414: LGTM!The aliasing of
privatetoisPrivatecorrectly avoids JavaScript reserved keyword issues, and the prop is properly passed to the Message component.Also applies to: 509-509
app/lib/database/model/Message.js (2)
91-91: LGTM!The WatermelonDB field decorator is correctly applied for the new
privatefield.
133-134: LGTM!The
privatefield is correctly included in theasPlain()serialization output, consistent with other fields in the model.app/containers/message/interfaces.ts (1)
120-120: LGTM!The addition of the optional
privatefield toIMessageInneris correctly typed and positioned within the interface.app/i18n/locales/en.json (1)
257-257: Note: Portuguese (pt-BR) translation pending.The English i18n strings for private messages have been added. As noted in the PR objectives (pending item #3), the pt-BR translations are still pending.
Also applies to: 616-616
app/views/RoomView/index.tsx (2)
867-869: LGTM!The guard correctly prevents long-press actions on private messages, maintaining the intended UX where private messages cannot trigger action sheets.
375-375: Verify fire-and-forget cleanup pattern for private messages.The
deletePrivateMessages()function is called withoutawaitin the asynccomponentWillUnmountlifecycle method. While the function has internal error handling and won't throw, the deletion may not complete before the component fully unmounts.If this fire-and-forget pattern is intentional (for performance), consider adding a comment. Otherwise, consider awaiting the cleanup:
- deletePrivateMessages(); + await deletePrivateMessages();app/lib/database/schema/app.js (1)
4-4: Migration for schema version 28 is correctly in place.The database migration exists in
app/lib/database/model/migrations.js(lines 336–343) with the properaddColumnsfor themessages.privatefield matching the schema update.app/containers/message/Message.tsx (1)
2-2: LGTM!The new imports are appropriate for the private message feature implementation.
Also applies to: 29-29
app/lib/database/model/migrations.js (1)
334-343: LGTM!The migration correctly adds the
privatecolumn to the messages table. The structure follows the existing pattern, version increments properly, and the boolean type withisOptional: trueis appropriate for this feature..maestro/tests/assorted/private-message.yaml (3)
1-18: LGTM!The test setup and lifecycle hooks follow best practices. User creation, room creation, login flow, and cleanup are properly configured.
53-84: LGTM!This test scenario correctly verifies the automatic cleanup behavior when navigating away from the room. The test steps are clear and the assertions properly validate that private messages are removed when returning to the room.
1-84: Excellent E2E test coverage for the private message feature.The test comprehensively covers:
- Manual dismissal of private messages
- Automatic cleanup when navigating away and returning
- Proper use of testIDs matching the implementation
- Clear scenario separation with comments
This aligns well with the PR objective: "Added e2e test to make sure it's working" ✓
app/lib/methods/deletePrivateMessages.ts (1)
10-12: The query column names are correct.
Q.where('id', id)properly uses the WatermelonDB-inherited primary key, andQ.where('private', true)correctly references the explicitly definedprivatefield in the Message model. No changes needed.
| {props.private && ( | ||
| <View style={styles.privateIndicator}> | ||
| <Text style={styles.privateIndicatorText}>{i18n.t('Only_you_can_see_this_message')} • </Text> | ||
| <Pressable testID='dismiss-private-message' onPress={() => deletePrivateMessages(props.id)}> | ||
| <Text style={styles.privateMessageDismiss}>{i18n.t('Dismiss_message')}</Text> | ||
| </Pressable> | ||
| </View> | ||
| )} |
There was a problem hiding this comment.
Add user feedback and error handling for dismiss action.
The dismiss action calls deletePrivateMessages without awaiting or providing feedback. Users won't know if the dismissal succeeded or failed.
Consider these improvements:
- Add a loading state while deleting
- Show success/error feedback (toast/snackbar)
- Disable the button during deletion to prevent multiple taps
- Handle potential errors gracefully
Example implementation:
+const [isDismissing, setIsDismissing] = React.useState(false);
+
+const handleDismiss = async () => {
+ if (isDismissing) return;
+ setIsDismissing(true);
+ try {
+ await deletePrivateMessages(props.id);
+ // Show success feedback (e.g., toast)
+ } catch (error) {
+ // Show error feedback
+ } finally {
+ setIsDismissing(false);
+ }
+};
{props.private && (
<View style={styles.privateIndicator}>
<Text style={styles.privateIndicatorText}>{i18n.t('Only_you_can_see_this_message')} • </Text>
- <Pressable testID='dismiss-private-message' onPress={() => deletePrivateMessages(props.id)}>
- <Text style={styles.privateMessageDismiss}>{i18n.t('Dismiss_message')}</Text>
+ <Pressable testID='dismiss-private-message' onPress={handleDismiss} disabled={isDismissing}>
+ <Text style={styles.privateMessageDismiss}>
+ {isDismissing ? i18n.t('Dismissing...') : i18n.t('Dismiss_message')}
+ </Text>
</Pressable>
</View>
)}Committable suggestion skipped: line range outside the PR's diff.
| await db.write(async () => { | ||
| try { | ||
| await db.batch(...messagesToBeDeleted); | ||
| } catch (e) { | ||
| console.log('e', e); | ||
| // Do nothing | ||
| } | ||
| }); |
There was a problem hiding this comment.
Improve error handling - don't silently swallow batch errors.
The inner try-catch at lines 16-21 swallows errors with only a console.log, making debugging difficult and preventing callers from knowing if deletion failed.
Consider these improvements:
await db.write(async () => {
- try {
- await db.batch(...messagesToBeDeleted);
- } catch (e) {
- console.log('e', e);
- // Do nothing
- }
+ await db.batch(...messagesToBeDeleted);
});If you need to handle specific batch errors, re-throw them so the outer catch can log properly:
await db.write(async () => {
try {
await db.batch(...messagesToBeDeleted);
} catch (e) {
- console.log('e', e);
- // Do nothing
+ log(e);
+ throw e; // Re-throw so caller knows deletion failed
}
});📝 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.
| await db.write(async () => { | |
| try { | |
| await db.batch(...messagesToBeDeleted); | |
| } catch (e) { | |
| console.log('e', e); | |
| // Do nothing | |
| } | |
| }); | |
| await db.write(async () => { | |
| await db.batch(...messagesToBeDeleted); | |
| }); |
| await db.write(async () => { | |
| try { | |
| await db.batch(...messagesToBeDeleted); | |
| } catch (e) { | |
| console.log('e', e); | |
| // Do nothing | |
| } | |
| }); | |
| await db.write(async () => { | |
| try { | |
| await db.batch(...messagesToBeDeleted); | |
| } catch (e) { | |
| log(e); | |
| throw e; // Re-throw so caller knows deletion failed | |
| } | |
| }); |
🤖 Prompt for AI Agents
In app/lib/methods/deletePrivateMessages.ts around lines 15 to 22 the inner
try-catch is swallowing batch errors (only doing console.log and continuing), so
replace that silent catch with proper error propagation: either remove the inner
try-catch entirely so a thrown error bubbles to the outer catch, or if you must
log here, log using the application's logger with a descriptive message and then
re-throw the error so the outer catch can handle/report it; ensure the logged
message includes the error object and context (which messages failed) instead of
console.log.
Proposed changes
This PR improve the private message behaviour
Pending thing:
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.