Skip to content

feat: Send webxdc status updates in InBroadcast-s (#7679) #7836

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.

Sign up for GitHub

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

Closed
iequidoo wants to merge 3 commits into from
Closed

feat: Send webxdc status updates in InBroadcast-s (#7679) #7836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0ffe81
feat: Don't send Chat-Group-Name* headers for InBroadcast-s
iequidoo Feb 6, 2026
de0b23d
feat: Send webxdc status updates in InBroadcast-s (#7679)
iequidoo Feb 6, 2026
12fdfc9
fix: Don't resend webxdc status updates in OutBroadcast-s (#7679)
iequidoo Feb 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/chat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2697,7 +2697,9 @@ async fn prepare_send_msg(
.unwrap_or_default(),
_ => false,
};
if let Some(reason) = chat.why_cant_send_ex(context, &skip_fn).await? {
if msg.param.get_cmd() == SystemMessage::WebxdcStatusUpdate {
// Already checked in `send_webxdc_status_update_struct()`.
} else if let Some(reason) = chat.why_cant_send_ex(context, &skip_fn).await? {
Comment on lines -2700 to +2702
Copy link
Copy Markdown
Collaborator

@Hocuri Hocuri Feb 11, 2026

Choose a reason for hiding this comment

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

Why not just add | SystemMessage::WebxdcStatusUpdate to this code above:

        CantSendReason::InBroadcast => {
            matches!(
                msg.param.get_cmd(),
                SystemMessage::MemberRemovedFromGroup | SystemMessage::SecurejoinMessage
            )
        }

?

Then the let skip_fn = |reason: &CantSendReason| *reason == CantSendReason::InBroadcast; change in webxdc.rs also wouldn't be necessary.

Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Feb 22, 2026

Choose a reason for hiding this comment

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

Then why_cant_send_ex() will be called twice and it's not a cheap function, so it's better to check everything early, before creating the status update record

bail!("Cannot send to {chat_id}: {reason}");
}

Expand Down Expand Up @@ -4680,7 +4682,7 @@ pub async fn resend_msgs(context: &Context, msg_ids: &[MsgId]) -> Result<()> {
// note(treefit): only matters if it is the last message in chat (but probably to expensive to check, debounce also solves it)
chatlist_events::emit_chatlist_item_changed(context, msg.chat_id);

if msg.viewtype == Viewtype::Webxdc {
if msg.viewtype == Viewtype::Webxdc && msg.chat_typ != Chattype::OutBroadcast {
let conn_fn = |conn: &mut rusqlite::Connection| {
let range = conn.query_row(
"SELECT IFNULL(min(id), 1), IFNULL(max(id), 0) \
Expand Down
2 changes: 2 additions & 0 deletions src/constants.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,14 @@ pub const DC_CHAT_ID_LAST_SPECIAL: ChatId = ChatId::new(9);
IntoStaticStr,
Serialize,
Deserialize,
Default,
)]
#[repr(u32)]
pub enum Chattype {
/// A 1:1 chat, i.e. a normal chat with a single contact.
///
/// Created by [`ChatId::create_for_contact`].
#[default]
Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Feb 8, 2026

Choose a reason for hiding this comment

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

This is to avoid adding Option<Chattype> to Message. We don't really have messages ouside of chats in the db.

Single = 100,

/// Group chat.
Expand Down
6 changes: 6 additions & 0 deletions src/message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@ pub struct Message {
pub(crate) is_dc_message: MessengerMessage,
pub(crate) original_msg_id: MsgId,
pub(crate) mime_modified: bool,
pub(crate) chat_typ: Chattype,
pub(crate) chat_visibility: ChatVisibility,
pub(crate) chat_blocked: Blocked,
pub(crate) location_id: u32,
Expand Down Expand Up @@ -527,6 +528,7 @@ impl Message {
m.param AS param,
m.hidden AS hidden,
m.location_id AS location,
c.type AS chat_typ,
c.archived AS visibility,
c.blocked AS blocked
FROM msgs m
Expand Down Expand Up @@ -586,6 +588,10 @@ impl Message {
param: row.get::<_, String>("param")?.parse().unwrap_or_default(),
hidden: row.get("hidden")?,
location_id: row.get("location")?,
// This is safe: see `ChatId::delete_ex()`, `None` chat type can't happen.
chat_typ: row
.get::<_, Option<_>>("chat_typ")?
.unwrap_or(Chattype::Single),
Comment on lines +591 to +594
Copy link
Copy Markdown
Collaborator

@Hocuri Hocuri Feb 11, 2026

Choose a reason for hiding this comment

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

I don't think it makes sense to load all kind of chat data every time we load a message. chat_visibility and chat_blocked also has only one usage each in the entire codebase (tests excluded), both of which could be easily removed.

Also, chat_typ is only needed when (re)sending messages, which doesn't happen as often as receiving messages.

Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Feb 22, 2026

Choose a reason for hiding this comment

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

But here it's cheap, until we remove JOIN at least. Also, why not have some context in the Message struct. All these chat_ fields are fixed-size, not even strings. We can put them into a substruct if they clutter Message

chat_visibility: row.get::<_, Option<_>>("visibility")?.unwrap_or_default(),
chat_blocked: row
.get::<_, Option<Blocked>>("blocked")?
Expand Down
28 changes: 15 additions & 13 deletions src/mimefactory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1589,10 +1589,7 @@ impl MimeFactory {
}
}

if chat.typ == Chattype::Group
|| chat.typ == Chattype::OutBroadcast
|| chat.typ == Chattype::InBroadcast
{
if chat.typ == Chattype::Group || chat.typ == Chattype::OutBroadcast {
headers.push((
"Chat-Group-Name",
mail_builder::headers::text::Text::new(chat.name.to_string()).into(),
Expand All @@ -1603,7 +1600,11 @@ impl MimeFactory {
mail_builder::headers::text::Text::new(ts.to_string()).into(),
));
}

}
if chat.typ == Chattype::Group
|| chat.typ == Chattype::OutBroadcast
|| chat.typ == Chattype::InBroadcast
{
match command {
SystemMessage::MemberRemovedFromGroup => {
let email_to_remove = msg.param.get(Param::Arg).unwrap_or_default();
Expand Down Expand Up @@ -2036,14 +2037,15 @@ impl MimeFactory {
HeaderDef::IrohGossipTopic.get_headername(),
mail_builder::headers::raw::Raw::new(topic).into(),
));
if let (Some(json), _) = context
.render_webxdc_status_update_object(
msg.id,
StatusUpdateSerial::MIN,
StatusUpdateSerial::MAX,
None,
)
.await?
if msg.chat_typ != Chattype::OutBroadcast
&& let (Some(json), _) = context
.render_webxdc_status_update_object(
msg.id,
StatusUpdateSerial::MIN,
StatusUpdateSerial::MAX,
None,
)
.await?
Copy link
Copy Markdown
Collaborator

@Hocuri Hocuri Feb 11, 2026

Choose a reason for hiding this comment

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

I'm not very used to the webxdc code, but: Doesn't this change mean that no status updates will be sent into OutBroadcasts at all anymore? And, isn't the change in src/chat.rs enough to prevent resending into OutBroadcasts?

Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Feb 22, 2026

Choose a reason for hiding this comment

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

Status updates will be sent, but only as separate messages (see flush_status_updates()), i.e. not with the instance initially. This is a bug of course and this needs a separate test/check

{
parts.push(context.build_status_update_part(&json));
}
Expand Down
13 changes: 9 additions & 4 deletions src/webxdc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use serde_json::Value;
use sha2::{Digest, Sha256};
use tokio::{fs::File, io::BufReader};

use crate::chat::{self, Chat};
use crate::chat::{self, CantSendReason, Chat};
use crate::constants::Chattype;
use crate::contact::ContactId;
use crate::context::Context;
Expand Down Expand Up @@ -536,9 +536,14 @@ impl Context {
let chat = Chat::load_from_db(self, chat_id)
.await
.with_context(|| format!("Failed to load chat {chat_id} from the database"))?;
if let Some(reason) = chat.why_cant_send(self).await.with_context(|| {
format!("Failed to check if webxdc update can be sent to chat {chat_id}")
})? {
let skip_fn = |reason: &CantSendReason| *reason == CantSendReason::InBroadcast;
if let Some(reason) = chat
.why_cant_send_ex(self, &skip_fn)
.await
.with_context(|| {
format!("Failed to check if webxdc update can be sent to chat {chat_id}")
})?
{
bail!("Cannot send to {chat_id}: {reason}.");
}

Expand Down
89 changes: 89 additions & 0 deletions src/webxdc/webxdc_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ use crate::chat::{
use crate::chatlist::Chatlist;
use crate::config::Config;
use crate::ephemeral;
use crate::imex::{BackupProvider, get_backup};
use crate::receive_imf::receive_imf;
use crate::securejoin::get_securejoin_qr;
use crate::test_utils::{E2EE_INFO_MSGS, TestContext, TestContextManager};
use crate::tools::{self, SystemTime};
use crate::{message, sql};
Expand Down Expand Up @@ -1588,6 +1590,93 @@ async fn test_webxdc_no_internet_access() -> Result<()> {
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_in_broadcast_send_status_update() -> Result<()> {
let mut tcm = TestContextManager::new();
let alice = &tcm.alice().await;
let bob = &tcm.bob().await;

let alice_chat_id = create_broadcast(alice, "bc".to_string()).await?;
let qr = get_securejoin_qr(alice, Some(alice_chat_id)).await?;
let bob_chat_id = tcm.exec_securejoin_qr(bob, alice, &qr).await;

let mut alice_instance = create_webxdc_instance(
alice,
"minimal.xdc",
include_bytes!("../../test-data/webxdc/minimal.xdc"),
)?;
alice_chat_id
.set_draft(alice, Some(&mut alice_instance))
.await?;
alice_instance = alice_chat_id.get_draft(alice).await?.unwrap();
alice
.send_webxdc_status_update(alice_instance.id, r#"{"payload":41}"#)
.await?;
send_msg(alice, alice_chat_id, &mut alice_instance).await?;
let sent_msg = alice.pop_sent_msg().await;
let bob_instance = bob.recv_msg(&sent_msg).await;
assert_eq!(bob_instance.chat_id, bob_chat_id);

let provider = BackupProvider::prepare(bob).await?;
let bob2 = &tcm.unconfigured().await;
get_backup(bob2, provider.qr()).await?;

bob.send_webxdc_status_update(bob_instance.id, r#"{"payload":42}"#)
.await?;
bob.flush_status_updates().await?;
// Don't wait to make sure that status updates are sent immediately, otherwise the check for
// Alice below isn't reliable.
let sent_msg = bob.pop_sent_msg_opt(Duration::ZERO).await.unwrap();
Copy link
Copy Markdown
Collaborator

@Hocuri Hocuri Feb 11, 2026

Choose a reason for hiding this comment

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

Why this change?

Copy link
Copy Markdown
Collaborator Author

@iequidoo iequidoo Feb 22, 2026

Choose a reason for hiding this comment

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

This is to make sure that status updates are sent immediately, otherwise the check below:

// See above for bob -- status updates are flushed immediately, no need to wait.
assert!(alice.pop_sent_msg_opt(Duration::ZERO).await.is_none());

i.e. that alice (the channel owner) doesn't resend subscribers' status updates isn't reliable.


alice.recv_msg_trash(&sent_msg).await;
assert_eq!(
alice
.get_webxdc_status_updates(alice_instance.id, StatusUpdateSerial(0))
.await?,
r#"[{"payload":41,"serial":1,"max_serial":2},
{"payload":42,"serial":2,"max_serial":2}]"#
);

bob2.recv_msg_trash(&sent_msg).await;
assert_eq!(
bob2.get_webxdc_status_updates(bob_instance.id, StatusUpdateSerial(0))
.await?,
r#"[{"payload":41,"serial":1,"max_serial":2},
{"payload":42,"serial":2,"max_serial":2}]"#
);

// Non-subscriber's status updates are rejected.
let alice_bob_id = alice.add_or_lookup_contact_id(bob).await;
remove_contact_from_chat(alice, alice_chat_id, alice_bob_id).await?;
alice.pop_sent_msg().await;
let status =
helper_send_receive_status_update(bob, alice, &bob_instance, &alice_instance).await?;
assert_eq!(
status,
r#"[{"payload":41,"serial":1,"max_serial":2},
{"payload":42,"serial":2,"max_serial":2}]"#
);

// Subscribers' status updates are confidential and shalln't be re-sent. So initial status
// updates aren't re-sent too.
let fiona = &tcm.fiona().await;
let fiona_chat_id = tcm.exec_securejoin_qr(fiona, alice, &qr).await;
resend_msgs(alice, &[alice_instance.id]).await?;
let sent1 = alice.pop_sent_msg().await;
alice.flush_status_updates().await?;
assert!(alice.pop_sent_msg_opt(Duration::ZERO).await.is_none());
let fiona_instance = fiona.recv_msg(&sent1).await;
assert_eq!(fiona_instance.chat_id, fiona_chat_id);
assert_eq!(fiona_instance.chat_typ, Chattype::InBroadcast);
assert_eq!(
fiona
.get_webxdc_status_updates(fiona_instance.id, StatusUpdateSerial(0))
.await?,
r#"[]"#
);
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_webxdc_chatlist_summary() -> Result<()> {
let t = TestContext::new_alice().await;
Expand Down
Loading