feat: add AB props cache and privacy tokens conditional - #442
Conversation
Introduce an in-memory AbPropsCache and populate it from PropsResponse on connect. Add well-known AB prop config codes and consult the cache to auto-attach privacy tokens on group create and participant add. Add a new AddParticipantsIq to serialize per-participant <privacy> tokens and export the ab_props store module.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds an in-memory A/B props cache and wires it into Client props fetching (apply responses before persisting hashes). Replaces AddParticipantsIq to carry per-participant options (including optional privacy bytes) and updates group create/add flows to conditionally attach tc_token-derived privacy based on A/B flags. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant Client
participant Cache as AbPropsCache
participant Storage as TokenStore
Server->>Client: PropsSpec Response (full or delta)
Client->>Cache: apply_response(&response)
Cache->>Cache: update/merge experiment props
Cache-->>Client: seeded / values available
Client->>Server: persist props hash
Note over Client,Cache: Later: Group operation triggered
Client->>Cache: is_enabled(PRIVACY_TOKEN_ON_GROUP_CREATE)
Cache-->>Client: true/false
alt enabled
Client->>Storage: lookup_valid_token(participant_jid)
Storage-->>Client: tc_token or None
Client->>Client: attach_tokens_to_participants()
end
Client->>Server: CreateGroupIq / AddParticipantsIq (with options)
sequenceDiagram
participant Client
participant Cache as AbPropsCache
participant Storage as TokenStore
participant Server
Client->>Cache: is_enabled(PRIVACY_TOKEN_ON_GROUP_PARTICIPANT_ADD)
Cache-->>Client: true/false
alt enabled
Client->>Client: resolve_participant_tokens(&jids)
loop per participant
Client->>Storage: lookup_valid_token(jid / resolved key)
Storage-->>Client: tc_token or None
Client->>Client: build GroupParticipantOptions (maybe privacy)
end
Client->>Server: AddParticipantsIq::with_options(group_jid, options_vec)
else disabled
Client->>Server: AddParticipantsIq::new(group_jid, &jids)
end
Server-->>Client: ParticipantChangeResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 21s —— View job Code Review
Overall this is a clean, well-structured PR. The AB props cache design is solid, tests are thorough, and the privacy token integration follows existing patterns. A few observations: Looks Good
Suggestions
Nits
Clean PR, good test coverage. The main actionable suggestion is #1 (avoiding the allocation in |
Bencher Report![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: d58b984867
ℹ️ 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 !response.delta_update { | ||
| map.clear(); |
There was a problem hiding this comment.
AbPropsCache starts empty on every process start, but Client::fetch_props() still requests deltas when a persisted props_hash exists. In that common reconnect path, apply_response() merges only changed props and does not repopulate unchanged ones, so is_enabled() can incorrectly return false for active flags until a full props refresh arrives. This breaks AB-gated behavior (e.g., group privacy token attachment) after restart even though the server config is enabled.
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: 3
🤖 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/client.rs`:
- Around line 1591-1593: The code is applying a delta response into an empty
AbPropsCache via self.ab_props.apply_response(&response).await which causes
unchanged props to disappear after restart; instead detect when the in-memory
AbPropsCache is unseeded/empty and avoid applying a delta: ensure fetch_props()
is called with a full spec (PropsSpec::new()) or otherwise request a full props
response until the AbPropsCache has been populated, or if a delta response is
received while AbPropsCache is empty, discard it and re-request a full props
response before calling AbPropsCache::apply_response; reference
self.ab_props.apply_response, fetch_props(), PropsSpec::new(), AbPropsCache, and
is_enabled() when making this change.
In `@wacore/src/iq/groups.rs`:
- Around line 1166-1187: In build_iq(), normalize the participant records before
serializing so phone_number is stripped for non-LID JIDs the same way
build_create_group_node() does: call normalize_participants(self.participants)
(or the existing normalization helper) and iterate over the normalized list when
building participant Nodes, ensuring phone_number is only emitted for LID JIDs;
update the mapping that builds participant Node attrs to use the normalized
participants and add a regression test that constructs a PN JID participant and
asserts no phone_number attribute is emitted in the produced IQ.
In `@wacore/src/store/ab_props.rs`:
- Around line 55-67: is_enabled currently conflates "prop is false" and "props
not yet loaded", causing callers to proceed incorrectly; change the API to
expose an uninitialized/tri-state instead of folding to false: either (A) change
pub async fn is_enabled(&self, config_code: u32) -> bool to return Option<bool>
(None = not primed, Some(true/false) = value), or (B) add a new pub async fn
is_enabled_opt(&self, config_code: u32) -> Option<bool> and keep is_enabled as a
convenience wrapper. Implement this by introducing a primed flag (e.g.,
self.props_primed or derive primed from a separate AtomicBool) that is set by
fetch_props(), and have is_enabled/_opt check that flag before reading
self.props (the symbols to update: is_enabled, fetch_props, connect, and the
props field). Update callers to handle None (block/refetch) where appropriate.
🪄 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: 0f5d6a11-2f39-41ad-b7ef-fc71544095df
📒 Files selected for processing (6)
src/client.rssrc/features/groups.rswacore/src/iq/groups.rswacore/src/iq/props.rswacore/src/store/ab_props.rswacore/src/store/mod.rs
| // Populate the in-memory AB props cache so features can query prop values. | ||
| self.ab_props.apply_response(&response).await; | ||
|
|
There was a problem hiding this comment.
Don't apply delta props onto a cold cache.
The device snapshot only persists props_hash, not the previous prop map. After a process restart, fetch_props() can request a delta with that hash and this call will merge the changed subset into an empty AbPropsCache, so unchanged props disappear and is_enabled() starts returning false for them. Use a full PropsSpec::new() until the in-memory cache has been seeded with a full response, or persist the full prop set alongside the hash.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client.rs` around lines 1591 - 1593, The code is applying a delta
response into an empty AbPropsCache via
self.ab_props.apply_response(&response).await which causes unchanged props to
disappear after restart; instead detect when the in-memory AbPropsCache is
unseeded/empty and avoid applying a delta: ensure fetch_props() is called with a
full spec (PropsSpec::new()) or otherwise request a full props response until
the AbPropsCache has been populated, or if a delta response is received while
AbPropsCache is empty, discard it and re-request a full props response before
calling AbPropsCache::apply_response; reference self.ab_props.apply_response,
fetch_props(), PropsSpec::new(), AbPropsCache, and is_enabled() when making this
change.
| /// Check if a prop is enabled (truthy). | ||
| /// | ||
| /// Returns `true` if the value is `"1"`, `"true"`, or `"enabled"` (case-insensitive). | ||
| /// Returns `false` if the prop is absent, empty, or has any other value. | ||
| pub async fn is_enabled(&self, config_code: u32) -> bool { | ||
| match self.props.read().await.get(&config_code) { | ||
| Some(value) => matches!( | ||
| value.to_ascii_lowercase().as_str(), | ||
| "1" | "true" | "enabled" | ||
| ), | ||
| None => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Expose an uninitialized state instead of folding it into false.
is_enabled() makes “flag disabled” and “props not fetched yet” indistinguishable. Because fetch_props() is started asynchronously during connect, the first group create/add after reconnect can still see an empty cache and skip privacy tokens, recreating the 463 path. Add a primed bit or a tri-state lookup so callers can block/refetch instead of silently taking the false branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/store/ab_props.rs` around lines 55 - 67, is_enabled currently
conflates "prop is false" and "props not yet loaded", causing callers to proceed
incorrectly; change the API to expose an uninitialized/tri-state instead of
folding to false: either (A) change pub async fn is_enabled(&self, config_code:
u32) -> bool to return Option<bool> (None = not primed, Some(true/false) =
value), or (B) add a new pub async fn is_enabled_opt(&self, config_code: u32) ->
Option<bool> and keep is_enabled as a convenience wrapper. Implement this by
introducing a primed flag (e.g., self.props_primed or derive primed from a
separate AtomicBool) that is set by fetch_props(), and have is_enabled/_opt
check that flag before reading self.props (the symbols to update: is_enabled,
fetch_props, connect, and the props field). Update callers to handle None
(block/refetch) where appropriate.
jlucaso1
changed the title
|
@claude review |
|
Claude finished @jlucaso1's task in 1m 10s —— View job Code Review
Overall this is a well-structured PR. The second commit (9a1b912) addressed the key cold-start delta issue flagged by reviewers and added the Looks Good
Issues
Nit
Clean PR with good test coverage. The main consideration is the race window in issue #1 — if |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a1b912880
ℹ️ 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".
| .ab_props() | ||
| .is_enabled(wacore::iq::props::config_codes::PRIVACY_TOKEN_ON_GROUP_CREATE) | ||
| .await | ||
| { |
There was a problem hiding this comment.
create_group now gates privacy-token attachment on ab_props().is_enabled(...), but the AB cache is populated asynchronously in background init (fetch_props is spawned after connect), so this check returns false on a fresh connection until that job completes. In AB-enabled deployments where group create/add requires privacy tokens, the first operations after wait_for_connected() can still be sent without tokens and fail, which undermines the feature this commit adds. Consider treating props fetch as readiness-critical for these flows or using a fallback that attaches a valid token when one exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wacore/src/store/ab_props.rs (1)
57-66:⚠️ Potential issue | 🟠 MajorDon't collapse "unseeded" into
false.
fetch_props()still runs asynchronously during startup, so callers can hit this method before the cache is primed. Returningfalsein that window still makes the first group create/add path skip privacy tokens and can recreate the 463 failure this PR is trying to fix. Expose a tri-state/readiness API here and make the group flows handle the unprimed case explicitly.Suggested shape
+ pub async fn is_enabled_opt(&self, config_code: u32) -> Option<bool> { + if !self.is_seeded() { + return None; + } + + let props = self.props.read().await; + Some(match props.get(&config_code) { + Some(value) => { + value == "1" + || value.eq_ignore_ascii_case("true") + || value.eq_ignore_ascii_case("enabled") + } + None => false, + }) + } + /// True when the prop value is truthy (`"1"`, `"true"`, or `"enabled"`). pub async fn is_enabled(&self, config_code: u32) -> bool { - match self.props.read().await.get(&config_code) { - Some(value) => { - value == "1" - || value.eq_ignore_ascii_case("true") - || value.eq_ignore_ascii_case("enabled") - } - None => false, - } + match self.is_enabled_opt(config_code).await { + Some(enabled) => enabled, + None => false, + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/store/ab_props.rs` around lines 57 - 66, The current is_enabled(&self, config_code: u32) -> bool collapses the uninitialized cache into false; change this to a tri-state/readiness API so callers can detect "unseeded" vs explicit false: add a new method (e.g., is_enabled_state or get_prop_state) that returns an enum (SeededTrue | SeededFalse | Unseeded) or Option<bool> and update code that calls is_enabled (notably the group create/add flows) to explicitly handle the Unseeded/None case by deferring action until fetch_props() has completed; keep fetch_props() and props.read() usage but ensure is_enabled no longer returns false for missing keys and update callers to check the readiness enum/option before skipping privacy token logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wacore/src/store/ab_props.rs`:
- Around line 57-66: The current is_enabled(&self, config_code: u32) -> bool
collapses the uninitialized cache into false; change this to a
tri-state/readiness API so callers can detect "unseeded" vs explicit false: add
a new method (e.g., is_enabled_state or get_prop_state) that returns an enum
(SeededTrue | SeededFalse | Unseeded) or Option<bool> and update code that calls
is_enabled (notably the group create/add flows) to explicitly handle the
Unseeded/None case by deferring action until fetch_props() has completed; keep
fetch_props() and props.read() usage but ensure is_enabled no longer returns
false for missing keys and update callers to check the readiness enum/option
before skipping privacy token logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ac6ab04-5f5e-46a0-a8fa-80f1be33cbdf
📒 Files selected for processing (5)
src/client.rssrc/features/groups.rswacore/src/iq/groups.rswacore/src/iq/props.rswacore/src/store/ab_props.rs
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 `@tests/e2e/src/lib.rs`:
- Around line 108-110: Update the stale inline comments to reflect actual
behavior: change wording that currently claims "PairSuccess + Connected" and
that wait_for_startup_sync waits for Connected — instead document that phase 1
accepts either PairSuccess or Connected (i.e., proceeds on either signal) and
that wait_for_startup_sync handles the critical app-state sync phase (not
specifically waiting for Connected). Locate and edit the comment blocks near the
references to PairSuccess/Connected and the wait_for_startup_sync mention
(search for wait_for_startup_sync and the "PairSuccess + Connected" text) and
make the comments accurate and concise.
🪄 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: bac09271-b127-4160-bf03-3271e736d852
📒 Files selected for processing (1)
tests/e2e/src/lib.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3198b1b35c
ℹ️ 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".
| .client | ||
| .execute(AddParticipantsIq::new(jid, participants)) | ||
| .await?; | ||
| .ab_props() | ||
| .is_enabled(wacore::iq::props::config_codes::PRIVACY_TOKEN_ON_GROUP_PARTICIPANT_ADD) | ||
| .await | ||
| { |
There was a problem hiding this comment.
add_participants gates privacy-token attachment on ab_props().is_enabled(...), but AB props are fetched asynchronously during background init after connect, so this check is false on a fresh session and the method sends AddParticipantsIq::new(...) without tokens. In deployments where participant-add requires privacy tokens, the first add operation right after wait_for_connected() can be rejected even though the feature is enabled server-side. This path needs the same readiness/fallback handling as create-group so uninitialized AB state does not silently disable token attachment.
Useful? React with 👍 / 👎.
1 participant
Introduce an in-memory AbPropsCache and populate it from PropsResponse on
connect. Add well-known AB prop config codes and consult the cache to auto-attach privacy tokens on group create and participant add. Add a new AddParticipantsIq to serialize per-participant tokens and export the ab_props store module.
Closes #435
Summary by CodeRabbit
New Features
Improvements
Tests/Chores