Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements contact deletion functionality #420

Merged
merged 37 commits into from
Jan 16, 2025
Merged

Conversation

MasterK0927
Copy link
Collaborator

Resolves #13

  • Adds the functionality for removing the contact from the contact list.

  • Adds a button on the create page (where contacts are listed) to allow users to remove contacts from the list.

@MasterK0927 MasterK0927 requested a review from mattyg January 8, 2025 11:22
@MasterK0927 MasterK0927 changed the title Implement contact deletion functionality Implements contact deletion functionality Jan 8, 2025
… removes the not needed event.stopPropagation()
@MasterK0927
Copy link
Collaborator Author

fill="none" is needed in delete icon for retaining its transparent background behr

ui/src/lib/svgIcons.ts Outdated Show resolved Hide resolved
ui/src/store/ContactStore.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattyg mattyg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested some changes inline.

One gotcha I want to make sure is handled (it may be handled already but we should confirm).

Say we have a private conversation with a contact, but we never received a profile for that contact agent and conversation. Then we delete the contact. What will be displayed as the profile name in the conversation messages, or the profile in the ConversationSummary, or in the title from the ConversationTitleStore?

@MasterK0927
Copy link
Collaborator Author

Requested some changes inline.

One gotcha I want to make sure is handled (it may be handled already but we should confirm).

Say we have a private conversation with a contact, but we never received a profile for that contact agent and conversation. Then we delete the contact. What will be displayed as the profile name in the conversation messages, or the profile in the ConversationSummary, or in the title from the ConversationTitleStore?

As per my understanding, we need to modify the delete behavior to maintain a "tombstone" record with basic display information even after deletion similar to how social media platforms handle deleted accounts.

Let me explain it in more detail.

Currently, when a contact is deleted, two key things happen:

  • Contact is removed from the contacts store.
  • cellId mapping for that contact is removed.

There is a potential edge case that we are surely missing. While the contact information is removed from the ContactStore , the messages in the private conversation would still exist and need to reference the sender's profile information.

If we never received a profile for that contact and then deleted the contact entry, we would lose the connection between the agent's public key and any profile information.

When trying to display profile information in conversation messages, ConversationSummary, or ConversationTitleStore, we would likely encounter one of these scenarios:

  1. Looking up profile information through the contact store
    The components would find nothing since the contact was deleted.

  2. Fetching profile information directly from the DHT
    There would be nothing to find since we never received a profile.

To properly handle this case, we could consider one of the following solutions:

  • Keep a permanent record of basic agent information separate from the contacts store.
    This could be a lightweight store that maintains a mapping of agent public keys to their last known display information, even after contact deletion.

  • Implement fallback display logic in UI components.
    This would show the agent's public key (truncated) when no profile information is available.

@MasterK0927 MasterK0927 requested a review from mattyg January 9, 2025 08:40
@mattyg
Copy link
Collaborator

mattyg commented Jan 9, 2025

When trying to display profile information in conversation messages, ConversationSummary, or ConversationTitleStore, we would likely encounter one of these scenarios:

Did you actually confirm this is the case? I think we may be handling it because currently messages are not included the readable for agents who do not have a profile. Not necessarily the ideal solution, but not terrible either.

If necessary I would prefer this option:

Implement fallback display logic in UI components.
This would show the agent's public key (truncated) when no profile information is available.

@MasterK0927
Copy link
Collaborator Author

When trying to display profile information in conversation messages, ConversationSummary, or ConversationTitleStore, we would likely encounter one of these scenarios:

Did you actually confirm this is the case? I think we may be handling it because currently messages are not included the readable for agents who do not have a profile. Not necessarily the ideal solution, but not terrible either.

If necessary I would prefer this option:

Implement fallback display logic in UI components.
This would show the agent's public key (truncated) when no profile information is available.

Yeah I am confirming that we have this issue, we are not actually removing the name of the contact deleted from the conversations.

Copy link
Collaborator

@mattyg mattyg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requested changes in comments inline

@MasterK0927 MasterK0927 requested a review from mattyg January 14, 2025 07:34
ui/src/lib/Dialog.svelte Outdated Show resolved Hide resolved
let pollInterval: NodeJS.Timeout;
$: hasAgentJoinedDht =
profiles !== undefined &&
$profiles?.list.find(([key]) => key === $contact.publicKeyB64) !== undefined;
$profiles?.list.find(([key]) => key === $contact?.publicKeyB64) !== undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this change is needed either anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ?. operator is needed here because when a contact is deleted, there's a brief moment where the contact data becomes undefined. Without ?., trying to read properties during that moment would cause an error that freezes the UI update.?. prevents this error, ensuring the deletion completes smoothly with instant UI feedback.

Copy link
Collaborator

@mattyg mattyg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline comments

@MasterK0927 MasterK0927 requested a review from mattyg January 14, 2025 20:12
@MasterK0927
Copy link
Collaborator Author

Dialog component in smaller viewportsScreenshot_2025-01-15-01-44-07-508_com.volla.messages-edit.jpg

…citly check if contact is undefined once, rather in map with ?., pass icon to Dialog action button
Copy link
Collaborator

@mattyg mattyg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a few minor changes to the implementation:

  • The dialog stays open while the contact is being deleted, and the "loading" indicator is shown in the dialog action button
  • Show the delete icon to the dialog action button
  • explicitly check if $contact is undefined once, rather than within the find callback
  • Added a period to the end of the string delete_contact_dialog_message
  • Added the feature to the changelog

Great work!

@mattyg mattyg merged commit 4c8edc3 into develop Jan 16, 2025
@mattyg mattyg deleted the feat/remove-contact branch January 16, 2025 02:38
mattyg added a commit that referenced this pull request Jan 16, 2025
Implements contact deletion functionality
@mattyg mattyg mentioned this pull request Jan 16, 2025
mattyg added a commit that referenced this pull request Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove a contact
2 participants