feat!: support LID and PN JIDs in is_on_whatsapp - #453
Conversation
BREAKING CHANGE: `is_on_whatsapp` now takes `&[Jid]` instead of `&[&str]`. `ContactInfo`, `ContactInfoSpec`, and `get_info()` are removed. Redesigns the IsOnWhatsApp usync query to match WA Web's ExistsJob: - Queries `<contact/>`, `<lid/>`, and `<business/>` protocols - Accepts both PN JIDs and LID JIDs (builds different user nodes per type) - Parses `pn_jid` attribute from response user nodes - Pre-populates known LIDs from cache on PN queries - Persists LID-PN mappings from results in both directions - IsOnWhatsAppResult now includes `lid`, `pn_jid`, and `is_business`
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughis_on_whatsapp now accepts Jid inputs, builds per-JID IsOnWhatsAppUser entries (with known_lid hints), issues separate PN and LID IsOnWhatsApp queries, merges IsOnWhatsAppResult vectors (now including pn_jid), persists LID↔PN mappings in two passes, and removes ContactInfo from exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Contacts as Contacts
participant LidCache as LidCache
participant WACoreIQ as WACoreIQ
participant Server as Server
participant Persist as Persist
Client->>Contacts: is_on_whatsapp(jids: &[Jid])
Contacts->>LidCache: lookup known_lid for PN JIDs
LidCache-->>Contacts: known_lid? (Option<String>)
Contacts->>WACoreIQ: send IsOnWhatsAppSpec(users, query_type=Pn)
Contacts->>WACoreIQ: send IsOnWhatsAppSpec(users, query_type=Lid) (if any)
WACoreIQ->>Server: IQ (PN: <contact>[+<lid>], LID: <user jid="...@lid">)
Server-->>WACoreIQ: IQ response (users with lid / pn_jid / contact nodes)
WACoreIQ-->>Contacts: Vec<IsOnWhatsAppResult> (includes pn_jid)
Contacts->>Persist: persist_lid_mappings(from results)
Contacts->>Persist: persist_pn_mappings(from pn_jid/lid pairs)
Persist-->>Contacts: ack
Contacts-->>Client: Vec<IsOnWhatsAppResult>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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 |
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 23s —— View job PR Review: feat!: support LID and PN JIDs in is_on_whatsapp
Overall this is a clean, well-structured PR. The WA Web ExistsJob alignment is solid, tests are thorough, and the breaking change is well-justified. A few observations: Minor Issues1. Duplicated parsing logic in
Consider reusing 2. After this PR, 3. .map(|r| (r.pn_jid.as_ref().unwrap(), Some(&r.jid))),This is safe because the preceding .filter_map(|r| {
if r.jid.is_lid() {
r.pn_jid.as_ref().map(|pn| (pn, Some(&r.jid)))
} else {
None
}
})Looks Good
Nit
LGTM with the optional |
![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: 8990ad59cb
ℹ️ 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".
| phone.clone() | ||
| .map(|user| { | ||
| if user.jid.is_pn() { | ||
| let phone = format!("+{}", user.jid.user); |
There was a problem hiding this comment.
build_user_nodes now always prefixes PN users with '+', so inputs like Jid::pn("+1234567890") (or parsed JIDs whose user part already contains +) are serialized as ++1234567890. The previous implementation explicitly handled already-prefixed numbers, so this change can produce malformed <contact> values and false negatives from is_on_whatsapp for callers that preserve E.164 formatting.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/contacts.rs`:
- Around line 86-92: Replace the filter + map + unwrap pattern by using
filter_map so you don't call unwrap(): in the call to persist_lid_mappings,
change the iterator chain on results.iter() to a single filter_map closure that
checks r.jid.is_lid() and then uses r.pn_jid.as_ref().map(...) to return
Some((pn, Some(&r.jid))) only when pn_jid is present; keep the target function
name persist_lid_mappings and the same tuple shape so the call site and types
remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 420b49f9-0886-4f15-9d0a-985abb54d254
📒 Files selected for processing (4)
src/features/contacts.rssrc/features/mod.rssrc/lib.rswacore/src/iq/usync.rs
Bencher Report- Use filter_map instead of filter + map + unwrap for LID-PN persist - Guard against double '+' prefix in PN user phone formatting
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/iq/usync.rs`:
- Around line 91-95: The public IsOnWhatsAppUser shape currently allows invalid
combinations; either change it to a typed enum (e.g. enum IsOnWhatsAppUser { Pn
{ jid: Jid, known_lid: Option<String> }, Lid { jid: Jid } }) so callers can only
construct the two valid variants, or add strict validation in
IsOnWhatsAppSpec::new to fail fast: allow only (a) a PN-type JID with optional
known_lid (and if known_lid is Some validate it equals the raw LID user part
format) or (b) a LID-type JID with known_lid == None; return an Err or panic on
any other combination. Ensure validation logic references IsOnWhatsAppUser and
IsOnWhatsAppSpec::new so unsupported usync payloads cannot be constructed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8acafd1-038d-46e3-a4fd-86370e952b6b
📒 Files selected for processing (2)
src/features/contacts.rswacore/src/iq/usync.rs
- Split PN and LID queries into separate requests with correct protocols: PN queries: <contact/>, <lid/>, <business><verified_name/></business> LID queries: <business><verified_name/></business> only - Add <verified_name/> child to <business/> query element - Parse <result> node for per-protocol errors before <list>
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 20s —— View job PR Review: feat!: support LID and PN JIDs in is_on_whatsapp
Well-structured PR that correctly aligns Remaining observations1.
2. The 3. Benchmark alert is unrelated — The Bencher alert on 4. PN wire format doc comment vs actual query protocols — The doc comment XML example for "LID query" at lines 27-38 shows Looks Good
LGTM with the optional doc comment fix. Nice work. |
Future fields (username, disappearing_mode, etc.) can be added without a breaking API change.
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/contacts.rs`:
- Around line 71-84: The loop currently treats any non-PN JID as a LID JID;
change the else branch to explicitly check jid.is_lid() and only push to
lid_users when true, otherwise log or skip unexpected JID types (e.g., warning
about group/newsletter JIDs) to make the contract explicit; use the existing
IsOnWhatsAppUser struct, the
self.client.lid_pn_cache.get_current_lid(&jid.user).await for PN handling, and
add a concise warning (e.g., warn!) referencing jid when an unexpected type is
encountered.
In `@wacore/src/iq/usync.rs`:
- Around line 336-344: The code that computes is_registered uses
contact_node.map(|c| c.attrs.get("type").is_some_and(|v| v == "in")) which is
inconsistent with the file's attrs().optional_string(...) pattern; update the
closure for contact_node to use c.attrs().optional_string("type").map_or(false,
|v| v == "in") (or equivalent) so parsing uses the same attrs().optional_string
API as elsewhere (ref: user_node, contact_node, is_registered).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13518437-9538-4246-a157-6d922f77bb06
📒 Files selected for processing (2)
src/features/contacts.rswacore/src/iq/usync.rs
| let contact_node = user_node.get_optional_child("contact"); | ||
| // LID queries omit contact protocol; presence in response implies registered (ExistsJob.js:70) | ||
| let is_registered = if jid.is_lid() && contact_node.is_none() { | ||
| true | ||
| } else { | ||
| contact_node | ||
| .map(|c| c.attrs.get("type").is_some_and(|v| v == "in")) | ||
| .unwrap_or(false) | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency in attribute access pattern.
Line 342 uses .attrs.get("type") directly while other parsing code in this file uses the attrs().optional_string() pattern (e.g., lines 317, 325-327, 332). This inconsistency doesn't affect functionality but reduces code uniformity.
♻️ Suggested consistency fix
let is_registered = if jid.is_lid() && contact_node.is_none() {
true
} else {
contact_node
- .map(|c| c.attrs.get("type").is_some_and(|v| v == "in"))
+ .map(|c| c.attrs().optional_string("type").is_some_and(|v| v == "in"))
.unwrap_or(false)
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/iq/usync.rs` around lines 336 - 344, The code that computes
is_registered uses contact_node.map(|c| c.attrs.get("type").is_some_and(|v| v ==
"in")) which is inconsistent with the file's attrs().optional_string(...)
pattern; update the closure for contact_node to use
c.attrs().optional_string("type").map_or(false, |v| v == "in") (or equivalent)
so parsing uses the same attrs().optional_string API as elsewhere (ref:
user_node, contact_node, is_registered).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ebaba5f25
ℹ️ 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".
| } else { | ||
| lid_users.push(IsOnWhatsAppUser { | ||
| jid: jid.to_non_ad(), | ||
| known_lid: None, | ||
| }); |
There was a problem hiding this comment.
is_on_whatsapp is documented to accept only PN and LID JIDs, but this branch treats every non-PN JID as a LID lookup candidate. That means inputs like group/newsletter/status JIDs get serialized into the LID usync request, which can produce malformed lookups and (in mixed batches) cause the whole call to error after a successful PN request. Adding an explicit jid.is_lid() check (and rejecting unsupported servers) would avoid false negatives and hard failures from accidental non-user inputs.
Useful? React with 👍 / 👎.
- Include <lid/> protocol in LID queries (WA Web adds it unconditionally) - Skip non-PN/non-LID JIDs with warning instead of silently treating them as LID queries - Extract parse_lid_jid() to deduplicate lid parsing across specs - Fix doc comments to match actual wire format
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c3b479b77
ℹ️ 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".
| if !pn_users.is_empty() { | ||
| let sid = self.client.generate_request_id(); | ||
| let spec = IsOnWhatsAppSpec::new(pn_users, sid, IsOnWhatsAppQueryType::Pn); | ||
| results.extend(self.client.execute(spec).await?); |
There was a problem hiding this comment.
is_on_whatsapp builds two independent batches and appends PN responses first, then LID responses, so mixed inputs can come back in a different order than requested (for example, [LID, PN] becomes [PN, LID]). Because this API returns a Vec, callers often associate entries by index with the original slice; reordering here can silently misattribute registration/business data to the wrong JID in mixed queries.
Useful? React with 👍 / 👎.
1 participant
Summary
is_on_whatsappnow takes&[Jid]instead of&[&str], supporting both PN (Jid::pn("123")) and LID (Jid::lid("100000001")) queries<contact/>,<lid/>,<business><verified_name/></business><lid/>,<business><verified_name/></business>pn_jidresponse attribute for LID-primary responses<result>node for per-protocol errors before<list>(WA Web parity)IsOnWhatsAppResultnow includeslid,pn_jid, andis_businessfields, marked#[non_exhaustive]for future-proofingget_info(),ContactInfo, andContactInfoSpec(zero callers;status/picture_idavailable viaget_user_info)Breaking Changes
is_on_whatsapp(&[&str])→is_on_whatsapp(&[Jid])IsOnWhatsAppResulthas new fields:lid,pn_jid,is_businessIsOnWhatsAppResultis now#[non_exhaustive]ContactInfotype removedget_info()method removedMigration
Test plan
cargo clippy --all --tests(zero warnings)cargo test --all --lib(all pass)Summary by CodeRabbit