feat: newsletter (channel) support — full CRUD, messaging, reactions, live updates - #390
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Newsletter feature: a crate-private Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Handle as NewsletterHandle
participant Mex as Mex/IQ Layer
participant Parser as Parser
participant EventSys as Event Dispatcher
Client->>Handle: newsletter().get_metadata(jid)
Handle->>Mex: send MEX GraphQL request (fetch metadata)
Mex-->>Handle: JSON/MEX response
Handle->>Parser: parse_newsletter_metadata(response)
Parser-->>Handle: NewsletterMetadata
Handle-->>Client: Result<NewsletterMetadata>
Mex->>EventSys: push IQ notification (newsletter live update)
EventSys->>Parser: parse live_updates/messages/reactions
Parser-->>EventSys: NewsletterLiveUpdate
EventSys-->>Client: emit Event::NewsletterLiveUpdate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Bencher Report
jlucaso1
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/newsletter.rs`:
- Around line 176-196: The current parsing silently maps missing/unknown fields
into defaults (name -> "", description -> None, subscriber_count -> 0,
verification -> NewsletterVerification::Unverified, state ->
NewsletterState::Active) which hides malformed payloads; update the
deserialization in the parsing block that handles thread and value so it fails
fast instead: validate presence and correct types for thread["name"]["text"],
thread["description"]["text"] (if required), thread["subscribers_count"],
thread["verification"], and value["state"]["type"], returning/propagating an
error (or Result::Err) when a required field is missing or contains an unknown
enum variant instead of mapping to empty/zero/defaults; specifically change the
code that sets name, description, subscriber_count, verification, and state to
use explicit checks (and map_str_to_enum logic) that return an error for unknown
verification -> NewsletterVerification and unknown state -> NewsletterState
rather than defaulting to Unverified/Active.
In `@tests/e2e/tests/newsletter.rs`:
- Around line 4-63: These two E2E tests (test_list_subscribed_newsletters and
test_get_newsletter_metadata) must be gated until the mock server supports
newsletter queries: modify the tests to early-skip instead of running against
the shared MOCK_SERVER_URL — either add a test-level gate (e.g., #[ignore] or a
cfg feature) or perform a runtime check after TestClient::connect(...) (check an
env var like NEWSLETTER_MOCK_ENABLED or a helper such as
client.mock_supports_newsletter()) and return Ok(()) when the mock lacks
newsletter support; update both test_list_subscribed_newsletters and
test_get_newsletter_metadata to use the chosen gate so they no longer run
against the unsupported mock server.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f04c3b6e-59e7-4a3b-b117-66eee2841c62
📒 Files selected for processing (7)
src/features/mod.rssrc/features/newsletter.rssrc/lib.rstests/e2e/tests/newsletter.rswacore/binary/src/jid.rswacore/src/iq/mod.rswacore/src/iq/newsletter.rs
Comment on lines
+176
to
+196
| let name = thread["name"]["text"].as_str().unwrap_or("").to_string(); | ||
| let description = thread["description"]["text"] | ||
| .as_str() | ||
| .filter(|s| !s.is_empty()) | ||
| .map(|s| s.to_string()); | ||
|
|
||
| let subscriber_count = thread["subscribers_count"] | ||
| .as_str() | ||
| .and_then(|s| s.parse::<u64>().ok()) | ||
| .unwrap_or(0); | ||
|
|
||
| let verification = match thread["verification"].as_str() { | ||
| Some("VERIFIED") => NewsletterVerification::Verified, | ||
| _ => NewsletterVerification::Unverified, | ||
| }; | ||
|
|
||
| let state = match value["state"]["type"].as_str() { | ||
| Some("suspended") => NewsletterState::Suspended, | ||
| Some("geosuspended") => NewsletterState::Geosuspended, | ||
| _ => NewsletterState::Active, | ||
| }; |
There was a problem hiding this comment.
Fail fast instead of manufacturing “valid” newsletter metadata from malformed payloads.
On Line 176, Line 182, and Line 192, missing or unknown server fields are collapsed into "", 0, Unverified, and Active. That hides schema drift and can incorrectly surface a suspended newsletter as active instead of failing the request.
🛠️ Example hardening
- let name = thread["name"]["text"].as_str().unwrap_or("").to_string();
+ let name = thread["name"]["text"]
+ .as_str()
+ .filter(|s| !s.is_empty())
+ .ok_or_else(|| MexError::PayloadParsing("missing newsletter name".into()))?
+ .to_string();
@@
- let subscriber_count = thread["subscribers_count"]
- .as_str()
- .and_then(|s| s.parse::<u64>().ok())
- .unwrap_or(0);
+ let subscriber_count = thread["subscribers_count"]
+ .as_u64()
+ .or_else(|| thread["subscribers_count"].as_str().and_then(|s| s.parse::<u64>().ok()))
+ .ok_or_else(|| {
+ MexError::PayloadParsing("missing newsletter subscriber count".into())
+ })?;Do the same for verification / state instead of defaulting unknown variants to a real state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/newsletter.rs` around lines 176 - 196, The current parsing
silently maps missing/unknown fields into defaults (name -> "", description ->
None, subscriber_count -> 0, verification -> NewsletterVerification::Unverified,
state -> NewsletterState::Active) which hides malformed payloads; update the
deserialization in the parsing block that handles thread and value so it fails
fast instead: validate presence and correct types for thread["name"]["text"],
thread["description"]["text"] (if required), thread["subscribers_count"],
thread["verification"], and value["state"]["type"], returning/propagating an
error (or Result::Err) when a required field is missing or contains an unknown
enum variant instead of mapping to empty/zero/defaults; specifically change the
code that sets name, description, subscriber_count, verification, and state to
use explicit checks (and map_str_to_enum logic) that return an error for unknown
verification -> NewsletterVerification and unknown state -> NewsletterState
rather than defaulting to Unverified/Active.
Comment on lines
+4
to
+63
| #[tokio::test] | ||
| async fn test_list_subscribed_newsletters() -> anyhow::Result<()> { | ||
| let _ = env_logger::builder().is_test(true).try_init(); | ||
|
|
||
| let client = TestClient::connect("e2e_newsletter_list").await?; | ||
|
|
||
| let newsletters = client.client.newsletter().list_subscribed().await?; | ||
| // Mock server should have at least one newsletter | ||
| assert!( | ||
| !newsletters.is_empty(), | ||
| "should have subscribed newsletters" | ||
| ); | ||
|
|
||
| let first = &newsletters[0]; | ||
| assert!(!first.name.is_empty(), "newsletter should have a name"); | ||
| assert!( | ||
| !first.jid.user.is_empty(), | ||
| "newsletter should have a JID user" | ||
| ); | ||
|
|
||
| info!( | ||
| "Listed {} subscribed newsletters, first: {} ({})", | ||
| newsletters.len(), | ||
| first.name, | ||
| first.jid | ||
| ); | ||
|
|
||
| client.disconnect().await; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_get_newsletter_metadata() -> anyhow::Result<()> { | ||
| let _ = env_logger::builder().is_test(true).try_init(); | ||
|
|
||
| let client = TestClient::connect("e2e_newsletter_meta").await?; | ||
|
|
||
| // First list to get a known newsletter JID | ||
| let newsletters = client.client.newsletter().list_subscribed().await?; | ||
| assert!(!newsletters.is_empty(), "need at least one newsletter"); | ||
|
|
||
| let jid = &newsletters[0].jid; | ||
| let metadata = client.client.newsletter().get_metadata(jid).await?; | ||
|
|
||
| assert_eq!(metadata.jid, *jid); | ||
| assert!(!metadata.name.is_empty(), "newsletter should have a name"); | ||
| assert!( | ||
| metadata.subscriber_count > 0, | ||
| "newsletter should have subscribers" | ||
| ); | ||
|
|
||
| info!( | ||
| "Fetched metadata for {}: name={}, subscribers={}", | ||
| metadata.jid, metadata.name, metadata.subscriber_count | ||
| ); | ||
|
|
||
| client.disconnect().await; | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Gate these E2E tests until the mock server implements newsletter queries.
On Line 8 and Line 40, the different TestClient::connect(...) prefixes only change local test isolation; both tests still point at the same MOCK_SERVER_URL. Since this PR explicitly calls out mock-server newsletter support as pending, these tests will fail for harness reasons as soon as the e2e suite starts running them.
🧪 Suggested fix
-#[tokio::test]
+#[tokio::test]
+#[ignore = "enable once the mock server supports newsletter GraphQL queries"]
async fn test_list_subscribed_newsletters() -> anyhow::Result<()> {
@@
-#[tokio::test]
+#[tokio::test]
+#[ignore = "enable once the mock server supports newsletter GraphQL queries"]
async fn test_get_newsletter_metadata() -> anyhow::Result<()> {📝 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.
Suggested change
| #[tokio::test] | |
| async fn test_list_subscribed_newsletters() -> anyhow::Result<()> { | |
| let _ = env_logger::builder().is_test(true).try_init(); | |
| let client = TestClient::connect("e2e_newsletter_list").await?; | |
| let newsletters = client.client.newsletter().list_subscribed().await?; | |
| // Mock server should have at least one newsletter | |
| assert!( | |
| !newsletters.is_empty(), | |
| "should have subscribed newsletters" | |
| ); | |
| let first = &newsletters[0]; | |
| assert!(!first.name.is_empty(), "newsletter should have a name"); | |
| assert!( | |
| !first.jid.user.is_empty(), | |
| "newsletter should have a JID user" | |
| ); | |
| info!( | |
| "Listed {} subscribed newsletters, first: {} ({})", | |
| newsletters.len(), | |
| first.name, | |
| first.jid | |
| ); | |
| client.disconnect().await; | |
| Ok(()) | |
| } | |
| #[tokio::test] | |
| async fn test_get_newsletter_metadata() -> anyhow::Result<()> { | |
| let _ = env_logger::builder().is_test(true).try_init(); | |
| let client = TestClient::connect("e2e_newsletter_meta").await?; | |
| // First list to get a known newsletter JID | |
| let newsletters = client.client.newsletter().list_subscribed().await?; | |
| assert!(!newsletters.is_empty(), "need at least one newsletter"); | |
| let jid = &newsletters[0].jid; | |
| let metadata = client.client.newsletter().get_metadata(jid).await?; | |
| assert_eq!(metadata.jid, *jid); | |
| assert!(!metadata.name.is_empty(), "newsletter should have a name"); | |
| assert!( | |
| metadata.subscriber_count > 0, | |
| "newsletter should have subscribers" | |
| ); | |
| info!( | |
| "Fetched metadata for {}: name={}, subscribers={}", | |
| metadata.jid, metadata.name, metadata.subscriber_count | |
| ); | |
| client.disconnect().await; | |
| Ok(()) | |
| #[tokio::test] | |
| #[ignore = "enable once the mock server supports newsletter GraphQL queries"] | |
| async fn test_list_subscribed_newsletters() -> anyhow::Result<()> { | |
| let _ = env_logger::builder().is_test(true).try_init(); | |
| let client = TestClient::connect("e2e_newsletter_list").await?; | |
| let newsletters = client.client.newsletter().list_subscribed().await?; | |
| // Mock server should have at least one newsletter | |
| assert!( | |
| !newsletters.is_empty(), | |
| "should have subscribed newsletters" | |
| ); | |
| let first = &newsletters[0]; | |
| assert!(!first.name.is_empty(), "newsletter should have a name"); | |
| assert!( | |
| !first.jid.user.is_empty(), | |
| "newsletter should have a JID user" | |
| ); | |
| info!( | |
| "Listed {} subscribed newsletters, first: {} ({})", | |
| newsletters.len(), | |
| first.name, | |
| first.jid | |
| ); | |
| client.disconnect().await; | |
| Ok(()) | |
| } | |
| #[tokio::test] | |
| #[ignore = "enable once the mock server supports newsletter GraphQL queries"] | |
| async fn test_get_newsletter_metadata() -> anyhow::Result<()> { | |
| let _ = env_logger::builder().is_test(true).try_init(); | |
| let client = TestClient::connect("e2e_newsletter_meta").await?; | |
| // First list to get a known newsletter JID | |
| let newsletters = client.client.newsletter().list_subscribed().await?; | |
| assert!(!newsletters.is_empty(), "need at least one newsletter"); | |
| let jid = &newsletters[0].jid; | |
| let metadata = client.client.newsletter().get_metadata(jid).await?; | |
| assert_eq!(metadata.jid, *jid); | |
| assert!(!metadata.name.is_empty(), "newsletter should have a name"); | |
| assert!( | |
| metadata.subscriber_count > 0, | |
| "newsletter should have subscribers" | |
| ); | |
| info!( | |
| "Fetched metadata for {}: name={}, subscribers={}", | |
| metadata.jid, metadata.name, metadata.subscriber_count | |
| ); | |
| client.disconnect().await; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/newsletter.rs` around lines 4 - 63, These two E2E tests
(test_list_subscribed_newsletters and test_get_newsletter_metadata) must be
gated until the mock server supports newsletter queries: modify the tests to
early-skip instead of running against the shared MOCK_SERVER_URL — either add a
test-level gate (e.g., #[ignore] or a cfg feature) or perform a runtime check
after TestClient::connect(...) (check an env var like NEWSLETTER_MOCK_ENABLED or
a helper such as client.mock_supports_newsletter()) and return Ok(()) when the
mock lacks newsletter support; update both test_list_subscribed_newsletters and
test_get_newsletter_metadata to use the chosen gate so they no longer run
against the unsupported mock server.
Collaborator
Author
|
@claude review |
Collaborator
Author
|
@codex review |
Collaborator
Author
|
@coderabbitai review |
|
Claude finished @jlucaso1's task in 1m 24s —— View job Code Review: Newsletter Feature PR
Overall this is a well-structured feature addition that follows the existing crate patterns nicely. The MEX/IQ split, the feature handle pattern, and the E2E test coverage are all solid. Here are the findings: Issues1.
2. Silent defaults in I partially agree with CodeRabbit here. The // Line 446-448: Unknown verification string silently becomes Unverified
let verification = match thread["verification"].as_str() {
Some("VERIFIED") => NewsletterVerification::Verified,
_ => NewsletterVerification::Unverified,
};
// Line 451-454: Unknown state string silently becomes Active
let state = match value["state"]["type"].as_str() {
Some("suspended") => NewsletterState::Suspended,
Some("geosuspended") => NewsletterState::Geosuspended,
_ => NewsletterState::Active,
};A suspended newsletter appearing as
The 3. Duplicated reaction parsing logic — The reaction parsing in 4. let server_id = msg_node
.attrs.get("server_id")
.map(|v| v.as_str())
.and_then(|s| s.parse::<u64>().ok())
.unwrap_or(0);A 5. let stanza = NodeBuilder::new("message")
.attr("to", jid.clone())
.attr("type", "text") // Always "text" even for media messagesThe method accepts any Minor / Nits6. let newsletter_jid = node.attrs().jid("from");If 7. E2E test assertions could be tighter — assert!(
page.len() <= 4,
"page before last should have fewer messages"
);This assertion ( 8. Missing
Looks Good
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 778fff93b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+354
to
+358
| let stanza = NodeBuilder::new("message") | ||
| .attr("to", jid.clone()) | ||
| .attr("type", "text") | ||
| .attr("id", &request_id) | ||
| .children([NodeBuilder::new("plaintext").bytes(encoded).build()]) |
There was a problem hiding this comment.
send_message() accepts any wa::Message, but this implementation always just wraps the protobuf in <plaintext> and sends it. That is fine for plain text, but newsletter image/video/document sends need the newsletter-specific upload metadata (including the unencrypted upload handle) and this crate currently has no path to attach it—src/upload.rs only implements the encrypted Client::upload() flow. As soon as a caller passes image_message, video_message, etc., the API will emit a malformed channel send even though the signature suggests those message types are supported.
Useful? React with 👍 / 👎.
Comment on lines
+1191
to
+1195
| if !messages.is_empty() { | ||
| client | ||
| .core | ||
| .event_bus | ||
| .dispatch(&Event::NewsletterLiveUpdate(NewsletterLiveUpdate { |
There was a problem hiding this comment.
Handling type="newsletter" here stops these stanzas from reaching the default Event::Notification path, but this branch only emits Event::NewsletterLiveUpdate. Existing consumers that watch raw notifications therefore stop seeing newsletter stanzas, and any newsletter notification fields that are not represented in NewsletterLiveUpdate are now dropped entirely. handle_group_notification() keeps the raw dispatch for backward compatibility, so this is a behavior regression specific to newsletter notifications.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/features/newsletter.rs (1)
439-459:⚠️ Potential issue | 🟠 MajorFail fast on malformed metadata instead of defaulting to real values.
Line 439-Line 459 currently collapse missing/unknown fields into valid-looking defaults (
"",0,Unverified,Active), which can hide payload/schema issues and misstate channel status.🛠️ Example hardening
- let name = thread["name"]["text"].as_str().unwrap_or("").to_string(); + let name = thread["name"]["text"] + .as_str() + .filter(|s| !s.is_empty()) + .ok_or_else(|| MexError::PayloadParsing("missing newsletter name".into()))? + .to_string(); - let subscriber_count = thread["subscribers_count"] - .as_str() - .and_then(|s| s.parse::<u64>().ok()) - .unwrap_or(0); + let subscriber_count = thread["subscribers_count"] + .as_u64() + .or_else(|| thread["subscribers_count"].as_str().and_then(|s| s.parse::<u64>().ok())) + .ok_or_else(|| MexError::PayloadParsing("missing newsletter subscriber count".into()))?; - let verification = match thread["verification"].as_str() { - Some("VERIFIED") => NewsletterVerification::Verified, - _ => NewsletterVerification::Unverified, - }; + let verification = match thread["verification"].as_str() { + Some("VERIFIED") => NewsletterVerification::Verified, + Some("UNVERIFIED") => NewsletterVerification::Unverified, + Some(v) => return Err(MexError::PayloadParsing(format!("unknown verification: {v}"))), + None => return Err(MexError::PayloadParsing("missing verification".into())), + }; - let state = match value["state"]["type"].as_str() { + let state = match value["state"]["type"].as_str() { Some("suspended") => NewsletterState::Suspended, Some("geosuspended") => NewsletterState::Geosuspended, - _ => NewsletterState::Active, + Some("active") => NewsletterState::Active, + Some(s) => return Err(MexError::PayloadParsing(format!("unknown state: {s}"))), + None => return Err(MexError::PayloadParsing("missing state.type".into())), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/newsletter.rs` around lines 439 - 459, The parsing currently masks malformed metadata by defaulting fields (name, description, subscriber_count, verification, state) to valid-looking values; change the logic in the newsletter parsing block to fail fast: validate that thread["name"]["text"] and thread["description"]["text"] exist and are non-empty (return Err or propagate Result) instead of using unwrap_or(""), parse subscriber_count with explicit error handling (propagate parse failure instead of defaulting to 0), and treat unknown verification or state strings as errors rather than mapping to NewsletterVerification::Unverified or NewsletterState::Active; update the function signature to return Result if needed, and replace the unwrap_or/and_then/default branches for name, description, subscriber_count, verification, and state with explicit matches that return an error on missing/invalid values so malformed payloads are surfaced immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/newsletter.rs`:
- Around line 320-337: The subscribe_live_updates function is using
client.send_iq with an owned Jid and bypasses the standard IQ path; change it to
use the client's execute(...) pattern with an IqSpec that takes &Jid (not Jid)
so the request goes through the standard IQ execution path: create an IqSpec (or
existing Spec type used project-wide) for NEWSLETTER_XMLNS and the
"live_updates" node using &jid, call self.client.execute(spec).await?, then
extract the "duration" from the returned response as before; apply the same
replacement to the other newsletter methods noted (the ones at lines 408-415) so
all newsletter IQs use client.execute(Spec::new(&jid)).await? and IqSpec
constructors that accept &Jid.
- Around line 553-565: The code currently synthesizes server_id and timestamp
from msg_node.attrs using unwrap_or(0), which produces ambiguous 0 values for
malformed nodes; change the parsing in the msg_node handling (the server_id and
timestamp extraction) to treat parse failures as invalid messages rather than
defaulting to 0 — return an error or skip/ignore the msg_node and emit a clear
log/metric when attrs.get("server_id") or attrs.get("t") is missing or fails to
parse to u64, and ensure downstream code that uses server_id/timestamp expects
that these fields may be absent (propagate Option<u64> or Result), so no
synthetic 0 values are stored or used for pagination/event correlation.
In `@src/handlers/notification.rs`:
- Around line 1140-1164: The code currently assigns server_id = 0 when parsing
fails, leaking bogus updates; change the mapping over children to skip malformed
message nodes by using filter_map (or an early return None) so you only
construct NewsletterLiveUpdateMessage when attrs.get("server_id") exists and
.parse::<u64>() succeeds; keep the reactions logic using parse_reaction_counts
and only collect NewsletterLiveUpdateMessage instances for which server_id was
valid (i.e., reference the closure that builds NewsletterLiveUpdateMessage, the
server_id parsing logic, and parse_reaction_counts) so invalid/malformed message
nodes are omitted instead of producing server_id = 0.
In `@tests/e2e/tests/newsletter.rs`:
- Around line 293-296: The assertion is too permissive given the request used
count=2; update the check on page.len() (in tests/e2e/tests/newsletter.rs) to
require the requested page size instead of <=4 — replace the current
assert!(page.len() <= 4, ...) with an assertion that page.len() == 2 (e.g.,
assert_eq!(page.len(), 2, "expected page to contain exactly count=2 items")) so
the test enforces the requested pagination size.
---
Duplicate comments:
In `@src/features/newsletter.rs`:
- Around line 439-459: The parsing currently masks malformed metadata by
defaulting fields (name, description, subscriber_count, verification, state) to
valid-looking values; change the logic in the newsletter parsing block to fail
fast: validate that thread["name"]["text"] and thread["description"]["text"]
exist and are non-empty (return Err or propagate Result) instead of using
unwrap_or(""), parse subscriber_count with explicit error handling (propagate
parse failure instead of defaulting to 0), and treat unknown verification or
state strings as errors rather than mapping to
NewsletterVerification::Unverified or NewsletterState::Active; update the
function signature to return Result if needed, and replace the
unwrap_or/and_then/default branches for name, description, subscriber_count,
verification, and state with explicit matches that return an error on
missing/invalid values so malformed payloads are surfaced immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08e846ed-7d00-4b6c-a476-e9373054d055
📒 Files selected for processing (7)
src/features/mod.rssrc/features/newsletter.rssrc/handlers/notification.rssrc/lib.rstests/e2e/tests/newsletter.rswacore/src/iq/newsletter.rswacore/src/types/events.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib.rs
- wacore/src/iq/newsletter.rs
Comment on lines
+320
to
+337
| pub async fn subscribe_live_updates(&self, jid: &Jid) -> Result<u64, anyhow::Error> { | ||
| let iq = InfoQuery::set( | ||
| NEWSLETTER_XMLNS, | ||
| jid.clone(), | ||
| Some(NodeContent::Nodes(vec![ | ||
| NodeBuilder::new("live_updates").build(), | ||
| ])), | ||
| ); | ||
|
|
||
| let response = self.client.send_iq(iq).await?; | ||
| let duration = response | ||
| .get_optional_child("live_updates") | ||
| .and_then(|n| n.attrs.get("duration")) | ||
| .map(|v| v.as_str()) | ||
| .and_then(|s| s.parse::<u64>().ok()) | ||
| .unwrap_or(300); | ||
|
|
||
| Ok(duration) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the standard IQ execution path for newsletter IQ operations.
These methods bypass the guideline-preferred IQ request path by calling send_iq directly with owned/cloned JIDs.
As per coding guidelines use client.execute(Spec::new(&jid)).await? pattern for IQ requests, with IqSpec constructors taking &Jid not Jid.
Also applies to: 408-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/newsletter.rs` around lines 320 - 337, The
subscribe_live_updates function is using client.send_iq with an owned Jid and
bypasses the standard IQ path; change it to use the client's execute(...)
pattern with an IqSpec that takes &Jid (not Jid) so the request goes through the
standard IQ execution path: create an IqSpec (or existing Spec type used
project-wide) for NEWSLETTER_XMLNS and the "live_updates" node using &jid, call
self.client.execute(spec).await?, then extract the "duration" from the returned
response as before; apply the same replacement to the other newsletter methods
noted (the ones at lines 408-415) so all newsletter IQs use
client.execute(Spec::new(&jid)).await? and IqSpec constructors that accept &Jid.
Comment on lines
+553
to
+565
| let server_id = msg_node | ||
| .attrs | ||
| .get("server_id") | ||
| .map(|v| v.as_str()) | ||
| .and_then(|s| s.parse::<u64>().ok()) | ||
| .unwrap_or(0); | ||
|
|
||
| let timestamp = msg_node | ||
| .attrs | ||
| .get("t") | ||
| .map(|v| v.as_str()) | ||
| .and_then(|s| s.parse::<u64>().ok()) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
Do not synthesize message IDs/timestamps from malformed nodes.
Defaulting invalid server_id/t to 0 can produce ambiguous records and break downstream pagination/event correlation.
🛠️ Suggested parser tightening
- for msg_node in children.iter().filter(|n| n.tag.as_ref() == "message") {
- let server_id = msg_node
+ for msg_node in children.iter().filter(|n| n.tag.as_ref() == "message") {
+ let Some(server_id) = msg_node
.attrs
.get("server_id")
.map(|v| v.as_str())
- .and_then(|s| s.parse::<u64>().ok())
- .unwrap_or(0);
+ .and_then(|s| s.parse::<u64>().ok()) else {
+ continue;
+ };
- let timestamp = msg_node
+ let Some(timestamp) = msg_node
.attrs
.get("t")
.map(|v| v.as_str())
- .and_then(|s| s.parse::<u64>().ok())
- .unwrap_or(0);
+ .and_then(|s| s.parse::<u64>().ok()) else {
+ continue;
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/newsletter.rs` around lines 553 - 565, The code currently
synthesizes server_id and timestamp from msg_node.attrs using unwrap_or(0),
which produces ambiguous 0 values for malformed nodes; change the parsing in the
msg_node handling (the server_id and timestamp extraction) to treat parse
failures as invalid messages rather than defaulting to 0 — return an error or
skip/ignore the msg_node and emit a clear log/metric when attrs.get("server_id")
or attrs.get("t") is missing or fails to parse to u64, and ensure downstream
code that uses server_id/timestamp expects that these fields may be absent
(propagate Option<u64> or Result), so no synthetic 0 values are stored or used
for pagination/event correlation.
Comment on lines
+293
to
+296
| assert!( | ||
| page.len() <= 4, | ||
| "page before last should have fewer messages" | ||
| ); |
There was a problem hiding this comment.
Tighten pagination assertions to match requested page size.
With count=2 (Line 290), asserting page.len() <= 4 is too permissive and can mask regressions.
🧪 Suggested assertion fix
- assert!(
- page.len() <= 4,
- "page before last should have fewer messages"
- );
+ assert!(page.len() <= 2, "page size should respect requested count");
+ assert!(
+ page.iter().all(|m| m.server_id < last_server_id),
+ "all paginated messages should be older than the cursor"
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/tests/newsletter.rs` around lines 293 - 296, The assertion is too
permissive given the request used count=2; update the check on page.len() (in
tests/e2e/tests/newsletter.rs) to require the requested page size instead of <=4
— replace the current assert!(page.len() <= 4, ...) with an assertion that
page.len() == 2 (e.g., assert_eq!(page.len(), 2, "expected page to contain
exactly count=2 items")) so the test enforces the requested pagination size.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/features/newsletter.rs (2)
500-523: Consider skipping reactions with emptycode.Reactions with empty
code(defaulting viaunwrap_or_default()) are likely malformed and could confuse downstream consumers. Afilterorfilter_mapapproach would be cleaner.♻️ Optional tightening
for r in children.iter().filter(|n| n.tag.as_ref() == "reaction") { - let code = r + let Some(code) = r .attrs .get("code") .map(|v| v.as_str().into_owned()) - .unwrap_or_default(); + .filter(|s| !s.is_empty()) + else { + continue; + }; let count = r .attrs .get("count")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/newsletter.rs` around lines 500 - 523, In parse_reaction_counts, skip reactions whose "code" attribute is empty instead of defaulting to an empty string: when iterating children in parse_reaction_counts, filter out entries where r.attrs.get("code") is missing or maps to an empty string (or use a filter_map to only collect Some(code) where code.trim().is_empty() is false), then parse the "count" and push NewsletterReactionCount { code, count } only for non-empty codes so malformed reactions are ignored by downstream consumers.
563-568: Timestamp still defaults to 0 for malformed nodes.While
server_idvalidation was improved (nodes without validserver_idare now skipped),timestampstill defaults to0when missing or unparseable. This can produce ambiguous records. Consider skipping or logging a warning for messages with invalid timestamps as well.♻️ Suggested tightening
- let timestamp = msg_node - .attrs - .get("t") - .map(|v| v.as_str()) - .and_then(|s| s.parse::<u64>().ok()) - .unwrap_or(0); + let Some(timestamp) = msg_node + .attrs + .get("t") + .map(|v| v.as_str()) + .and_then(|s| s.parse::<u64>().ok()) + else { + log::debug!("Skipping newsletter message with missing/invalid timestamp"); + continue; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/newsletter.rs` around lines 563 - 568, The timestamp extraction currently falls back to 0 for missing/unparseable values (the let timestamp = msg_node.attrs.get("t").map(...).and_then(...).unwrap_or(0) line), which creates ambiguous records; instead, detect when msg_node.attrs.get("t") is absent or s.parse::<u64>() fails and either log a warning via your logger and skip processing this msg_node or return/continue early so malformed timestamps are not recorded—update the code that handles timestamp parsing to avoid unwrap_or(0) and use an explicit match/if-let to handle Err/None paths (mirroring the server_id validation flow) and reference the same logging/skip behavior used for invalid server_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/notification.rs`:
- Line 1134: Replace the direct call to node.attrs().jid("from") with the
defensive pattern used elsewhere: call optional_jid on node.attrs() to obtain
newsletter_jid and return early if it is None (similar to
handle_picture_notification). Specifically, locate the expression
node.attrs().jid("from") and change the logic to use optional_jid(node.attrs(),
"from") (or the existing optional_jid helper with the same semantics), assign
the result to newsletter_jid only if present, and perform an early return when
newsletter_jid is missing to avoid panics.
---
Nitpick comments:
In `@src/features/newsletter.rs`:
- Around line 500-523: In parse_reaction_counts, skip reactions whose "code"
attribute is empty instead of defaulting to an empty string: when iterating
children in parse_reaction_counts, filter out entries where r.attrs.get("code")
is missing or maps to an empty string (or use a filter_map to only collect
Some(code) where code.trim().is_empty() is false), then parse the "count" and
push NewsletterReactionCount { code, count } only for non-empty codes so
malformed reactions are ignored by downstream consumers.
- Around line 563-568: The timestamp extraction currently falls back to 0 for
missing/unparseable values (the let timestamp =
msg_node.attrs.get("t").map(...).and_then(...).unwrap_or(0) line), which creates
ambiguous records; instead, detect when msg_node.attrs.get("t") is absent or
s.parse::<u64>() fails and either log a warning via your logger and skip
processing this msg_node or return/continue early so malformed timestamps are
not recorded—update the code that handles timestamp parsing to avoid
unwrap_or(0) and use an explicit match/if-let to handle Err/None paths
(mirroring the server_id validation flow) and reference the same logging/skip
behavior used for invalid server_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9612dc1-b588-4ee9-9685-7fc0ead41069
📒 Files selected for processing (3)
src/features/newsletter.rssrc/handlers/notification.rstests/e2e/tests/newsletter.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/tests/newsletter.rs
Add newsletter feature module with read-only Mex GraphQL operations: - list_subscribed() — list all subscribed newsletters - get_metadata(&Jid) — fetch newsletter metadata by JID - get_metadata_by_invite(&str) — fetch metadata by invite code New types: NewsletterMetadata, NewsletterVerification, NewsletterState, NewsletterRole, exposed via client.newsletter() feature handle. Also adds: - Jid::newsletter() factory method for zero-alloc JID construction - Mex document ID constants for all newsletter GraphQL operations - E2E test stubs for list_subscribed and get_metadata
- Add create(name, description) via Mex mutation (doc 25149874324715067) - Add join(jid) via Mex mutation (doc 24404358912487870) - Rewrite e2e tests to: create → list → get_metadata → get_by_invite → join - All 3 e2e tests pass against mock server
- Add leave(jid) via Mex mutation (doc 9767147403369991) - Add update(jid, name, description) via Mex mutation (doc 24250201037901610) - Add LEAVE doc ID constant - E2E tests: 5/5 passing (create, list, metadata, join, leave, update)
- send_message(jid, message) — plaintext stanza (no Signal encryption) - get_messages(jid, count, before) — IQ xmlns="newsletter" with server_id pagination cursor - NewsletterMessage type with decoded protobuf + reaction counts - Leave mutation doc ID (9767147403369991, from WA Web JS) - E2E tests: 7/7 passing (send, history, pagination, all CRUD ops)
- subscribe_live_updates(jid) — IQ SET xmlns="newsletter" <live_updates/> - send_reaction(jid, server_id, emoji) — reaction message stanza - NewsletterLiveUpdate event with per-message reaction counts - Notification handler for type="newsletter" parses <live_updates> children and dispatches Event::NewsletterLiveUpdate - E2E tests: 9/9 passing (full lifecycle including reaction live updates)
- Re-export NewsletterMessage and NewsletterReactionCount from crate root - Extract shared parse_reaction_counts() helper, used by both message history parser and notification handler (DRY) - Dispatch raw Event::Notification for newsletter notifications alongside Event::NewsletterLiveUpdate for backward compatibility (matches group handler pattern) - Document media upload limitation on send_message()
- Skip message nodes without valid server_id instead of defaulting to 0 (both in history parser and notification handler) - Tighten pagination test assertion from <= 4 to <= 2 (matches count=2)
- Use optional_jid("from") with early return instead of jid("from")
which returns Jid::default() on missing attrs (matches other handlers)
- Skip reactions with empty code attribute instead of defaulting to ""
jlucaso1
force-pushed
the
feat/newsletter-phase1
branch
from
March 18, 2026 19:54
44272c9 to
74d8eea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/features/newsletter.rs (3)
566-571:⚠️ Potential issue | 🟡 MinorAvoid synthesizing
timestamp = 0for malformed message nodes.Using
0creates ambiguous records and weakens downstream ordering/correlation. Skip invalid nodes (or make timestamp optional) instead of manufacturing a value.Suggested change
- let timestamp = msg_node - .attrs - .get("t") - .map(|v| v.as_str()) - .and_then(|s| s.parse::<u64>().ok()) - .unwrap_or(0); + let Some(timestamp) = msg_node + .attrs + .get("t") + .map(|v| v.as_str()) + .and_then(|s| s.parse::<u64>().ok()) + else { + continue; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/newsletter.rs` around lines 566 - 571, The code currently assigns timestamp = 0 when parsing msg_node.attrs.get("t") fails; instead, change the handling in the parsing logic around msg_node to skip malformed nodes or make the timestamp an Option<u64> by returning None when .parse::<u64>().ok() is None; locate the block that builds timestamp (the chain using msg_node.attrs.get("t").map(|v| v.as_str()).and_then(|s| s.parse::<u64>().ok()).unwrap_or(0)) and remove the unwrap_or(0), either filtering out the msg_node when the parse returns None or propagating Option<u64> (e.g., timestamp_opt) so downstream code can handle missing timestamps explicitly.
320-337:⚠️ Potential issue | 🟠 MajorUse the standard IQ execution path for newsletter IQ methods.
subscribe_live_updatesandget_messagesare still bypassing the project’s preferred IQ pipeline viasend_iq+ owned/cloned JIDs. Please route these through the standardexecute(Spec...)path with&Jidto keep behavior and middleware handling consistent.As per coding guidelines: "Use
client.execute(Spec::new(&jid)).await?pattern for IQ requests, with IqSpec constructors taking&JidnotJid."Also applies to: 408-415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/newsletter.rs` around lines 320 - 337, The subscribe_live_updates function (and similarly get_messages) currently builds an InfoQuery and calls self.client.send_iq with an owned/cloned Jid; change this to use the project's IQ pipeline by creating the appropriate IqSpec (or IqSpec::new/InfoQuerySpec) that accepts a &Jid and passing it to self.client.execute(spec).await? instead of send_iq, and stop cloning/owning the Jid—use &jid throughout so middleware, timeouts, and handlers are applied consistently via execute(Spec::new(&jid)). Ensure the spec constructor you use mirrors the set/query from InfoQuery::set but takes &Jid as per the IqSpec constructors.
439-459:⚠️ Potential issue | 🟠 MajorFail fast on malformed metadata instead of defaulting to valid-looking values.
Defaulting
name,subscriber_count,verification, andstatemasks payload/schema issues and can surface incorrect newsletter state (e.g., unknown state treated asActive).Suggested hardening
- let name = thread["name"]["text"].as_str().unwrap_or("").to_string(); + let name = thread["name"]["text"] + .as_str() + .filter(|s| !s.is_empty()) + .ok_or_else(|| MexError::PayloadParsing("missing newsletter name".into()))? + .to_string(); - let subscriber_count = thread["subscribers_count"] - .as_str() - .and_then(|s| s.parse::<u64>().ok()) - .unwrap_or(0); + let subscriber_count = thread["subscribers_count"] + .as_u64() + .or_else(|| thread["subscribers_count"].as_str().and_then(|s| s.parse::<u64>().ok())) + .ok_or_else(|| MexError::PayloadParsing("missing/invalid subscribers_count".into()))?; - let verification = match thread["verification"].as_str() { - Some("VERIFIED") => NewsletterVerification::Verified, - _ => NewsletterVerification::Unverified, - }; + let verification = match thread["verification"].as_str() { + Some("VERIFIED") => NewsletterVerification::Verified, + Some("UNVERIFIED") => NewsletterVerification::Unverified, + Some(other) => return Err(MexError::PayloadParsing(format!("unknown verification: {other}"))), + None => return Err(MexError::PayloadParsing("missing verification".into())), + }; - let state = match value["state"]["type"].as_str() { - Some("suspended") => NewsletterState::Suspended, - Some("geosuspended") => NewsletterState::Geosuspended, - _ => NewsletterState::Active, - }; + let state = match value["state"]["type"].as_str() { + Some("active") => NewsletterState::Active, + Some("suspended") => NewsletterState::Suspended, + Some("geosuspended") => NewsletterState::Geosuspended, + Some(other) => return Err(MexError::PayloadParsing(format!("unknown state: {other}"))), + None => return Err(MexError::PayloadParsing("missing state".into())), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/newsletter.rs` around lines 439 - 459, The current parsing silently defaults malformed/missing fields (name, subscriber_count, verification, state) which masks payload/schema errors; update the parsing in the newsletter deserialization to fail fast by returning an Err when required fields are missing or when values are unrecognized: for thread["name"]["text"] and thread["description"]["text"] validate presence and non-empty (return Err on missing required name), parse thread["subscribers_count"] using .as_str() and .parse::<u64>() and return Err if parsing fails, and for thread["verification"] and value["state"]["type"] map only known strings to NewsletterVerification and NewsletterState and return Err for any unknown/absent variant so callers can handle malformed payloads instead of silently using NewsletterVerification::Unverified or NewsletterState::Active.
🧹 Nitpick comments (1)
src/features/newsletter.rs (1)
244-251: Add a guard for empty newsletter updates.When both
nameanddescriptionareNone, this sends an emptyupdatesobject. Consider failing fast client-side with a clear validation error.Suggested guard
let mut updates = json!({}); if let Some(name) = name { updates["name"] = json!(name); } if let Some(desc) = description { updates["description"] = json!(desc); } + if updates.as_object().is_some_and(|m| m.is_empty()) { + return Err(MexError::PayloadParsing( + "update requires at least one field (name or description)".into(), + )); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/newsletter.rs` around lines 244 - 251, The handler currently builds an empty JSON patch object (`let mut updates = json!({});`) and proceeds even when both `name` and `description` are None; add a guard that checks for no-op updates (i.e., both `name` and `description` are None or `updates` remains empty) and return a clear client validation error instead of sending an empty update. Locate the block around `let mut updates = json!({});` and the `if let Some(name)` / `if let Some(desc)` branches and add an early return (e.g., Err/HttpResponse::BadRequest) with a message like "no updates provided" when nothing changed. Ensure the returned error uses the same error type/response conventions as the surrounding function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/notification.rs`:
- Around line 1134-1136: The code currently returns early when
node.attrs().optional_jid("from") is None, which prevents the
backward-compatible raw notification path; instead, when newsletter_jid is
missing call the raw dispatch path by emitting Event::Notification (or invoking
the existing raw notification dispatch function) before returning. Concretely,
in the notification handling block where you call
node.attrs().optional_jid("from") (and bind newsletter_jid), replace the early
return with code that constructs and dispatches an Event::Notification fallback
(using the same payload the raw path expects) and then return; ensure both
places noted (the current block and the similar block around the other
occurrence) follow the same pattern so missing `from` triggers the raw
Event::Notification dispatch rather than skipping it.
---
Duplicate comments:
In `@src/features/newsletter.rs`:
- Around line 566-571: The code currently assigns timestamp = 0 when parsing
msg_node.attrs.get("t") fails; instead, change the handling in the parsing logic
around msg_node to skip malformed nodes or make the timestamp an Option<u64> by
returning None when .parse::<u64>().ok() is None; locate the block that builds
timestamp (the chain using msg_node.attrs.get("t").map(|v|
v.as_str()).and_then(|s| s.parse::<u64>().ok()).unwrap_or(0)) and remove the
unwrap_or(0), either filtering out the msg_node when the parse returns None or
propagating Option<u64> (e.g., timestamp_opt) so downstream code can handle
missing timestamps explicitly.
- Around line 320-337: The subscribe_live_updates function (and similarly
get_messages) currently builds an InfoQuery and calls self.client.send_iq with
an owned/cloned Jid; change this to use the project's IQ pipeline by creating
the appropriate IqSpec (or IqSpec::new/InfoQuerySpec) that accepts a &Jid and
passing it to self.client.execute(spec).await? instead of send_iq, and stop
cloning/owning the Jid—use &jid throughout so middleware, timeouts, and handlers
are applied consistently via execute(Spec::new(&jid)). Ensure the spec
constructor you use mirrors the set/query from InfoQuery::set but takes &Jid as
per the IqSpec constructors.
- Around line 439-459: The current parsing silently defaults malformed/missing
fields (name, subscriber_count, verification, state) which masks payload/schema
errors; update the parsing in the newsletter deserialization to fail fast by
returning an Err when required fields are missing or when values are
unrecognized: for thread["name"]["text"] and thread["description"]["text"]
validate presence and non-empty (return Err on missing required name), parse
thread["subscribers_count"] using .as_str() and .parse::<u64>() and return Err
if parsing fails, and for thread["verification"] and value["state"]["type"] map
only known strings to NewsletterVerification and NewsletterState and return Err
for any unknown/absent variant so callers can handle malformed payloads instead
of silently using NewsletterVerification::Unverified or NewsletterState::Active.
---
Nitpick comments:
In `@src/features/newsletter.rs`:
- Around line 244-251: The handler currently builds an empty JSON patch object
(`let mut updates = json!({});`) and proceeds even when both `name` and
`description` are None; add a guard that checks for no-op updates (i.e., both
`name` and `description` are None or `updates` remains empty) and return a clear
client validation error instead of sending an empty update. Locate the block
around `let mut updates = json!({});` and the `if let Some(name)` / `if let
Some(desc)` branches and add an early return (e.g.,
Err/HttpResponse::BadRequest) with a message like "no updates provided" when
nothing changed. Ensure the returned error uses the same error type/response
conventions as the surrounding function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dab9705-3c59-41a2-8a0d-efca3ebd85ad
📒 Files selected for processing (9)
src/features/mod.rssrc/features/newsletter.rssrc/handlers/notification.rssrc/lib.rstests/e2e/tests/newsletter.rswacore/binary/src/jid.rswacore/src/iq/mod.rswacore/src/iq/newsletter.rswacore/src/types/events.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/features/mod.rs
- tests/e2e/tests/newsletter.rs
- wacore/src/types/events.rs
- src/lib.rs
Comment on lines
+1134
to
+1136
| let Some(newsletter_jid) = node.attrs().optional_jid("from") else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Preserve raw notification fallback when from is missing.
The early return on missing from skips the backward-compat raw dispatch path. Consider dispatching Event::Notification before returning.
Suggested fix
let Some(newsletter_jid) = node.attrs().optional_jid("from") else {
+ warn!(target: "Client/Newsletter", "newsletter notification missing 'from' attribute");
+ client
+ .core
+ .event_bus
+ .dispatch(&Event::Notification(node.clone()));
return;
};Also applies to: 1178-1183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handlers/notification.rs` around lines 1134 - 1136, The code currently
returns early when node.attrs().optional_jid("from") is None, which prevents the
backward-compatible raw notification path; instead, when newsletter_jid is
missing call the raw dispatch path by emitting Event::Notification (or invoking
the existing raw notification dispatch function) before returning. Concretely,
in the notification handling block where you call
node.attrs().optional_jid("from") (and bind newsletter_jid), replace the early
return with code that constructs and dispatches an Event::Notification fallback
(using the same payload the raw path expects) and then return; ensure both
places noted (the current block and the similar block around the other
occurrence) follow the same pattern so missing `from` triggers the raw
Event::Notification dispatch rather than skipping it.
3 tasks
This was referenced Jun 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full newsletter (channel) feature implementation. WhatsApp calls these "Channels" in the UI but "newsletter" in the protocol layer.
API
New types
NewsletterMetadata— full metadata (name, description, subscribers, verification, invite code, etc.)NewsletterMessage— history entry with decoded protobuf + reaction countsNewsletterReactionCount— emoji + countNewsletterVerification,NewsletterState,NewsletterRoleNewsletterLiveUpdate,NewsletterLiveUpdateMessage,NewsletterLiveUpdateReaction(events)Jid::newsletter(id)factory methodNew files
wacore/src/iq/newsletter.rs— Mex doc ID constants for all newsletter GraphQL operationssrc/features/newsletter.rs— Feature handle with 11 methods + typessrc/handlers/notification.rs— Newsletter notification handler (type="newsletter")wacore/src/types/events.rs—Event::NewsletterLiveUpdatevarianttests/e2e/tests/newsletter.rs— 9 E2E testsE2E tests (9/9 passing)
test_newsletter_create_and_listtest_newsletter_get_metadatatest_newsletter_jointest_newsletter_leavetest_newsletter_updatetest_newsletter_send_and_get_messagestest_newsletter_message_paginationbeforecursortest_newsletter_subscribe_live_updatestest_newsletter_reaction_live_updateEvent::NewsletterLiveUpdatewith reaction countsNot covered (can be added later)
These are niche admin-only features. Mex doc IDs are already defined in
wacore/src/iq/newsletter.rs:fetch_admin_count— get admin count for a newsletterfetch_admin_capabilities— get admin permissionsfetch_pending_invites— get pending admin invitationsfetch_subscribers— get full subscriber list (admin only)fetch_reaction_senders— get who reacted with each emojiTest plan
cargo test --all --exclude e2e-tests— all unit tests passcargo clippy --all --tests— zero warningscargo test -p e2e-tests --test newsletter— 9/9 passingSummary by CodeRabbit