feat(abprops): vendor typed A/B-props registry, drop hand-maintained config_codes - #729
Merged
Conversation
…config_codes
Replace the hand-curated `config_codes` (11 bare `u32`s) with the whatspec-generated typed registry, vendored as `wacore::iq::abprops` (modules `web`/`hybrid`/`group`, 2100 flags). Each flag is a `pub const AbProp { name, code, value_type, default }`. Refresh by re-copying whatspec's `generated/abprops/abprops.rs` and running `cargo fmt`.
Zero binary cost for the unused flags: `const`s emit no symbol unless referenced (not even LTO-dependent), and nothing references the per-module or top-level `ALL` aggregates, so only the flags in `props::WATCHED` (and their name strings) land in the binary.
The cache getters (`get`/`is_enabled`/`is_enabled_or`/`get_int`/`watch`) now take a typed `AbProp` instead of a magic `u32`; callsites reference named flags (`abprops::web::PRIVACY_TOKEN_SENDING_ON_GROUP_CREATE`, ...). `apply_props` stays keyed by the wire `u32`. `props::WATCHED` (the 11 flags this client reads) seeds the interest set, replacing `config_codes::ALL`.
Two flags the current WA Web bundle no longer ships (`privacy_token_only_check_lid` 15491, `profile_pic_privacy_token` 9666) are absent from the registry; they live in `props::stale` as explicit consts to preserve behavior (the server never sends them, so gating always falls to the callsite default).
`LID_TRUSTED_TOKEN_ISSUE_TO_LID` now follows the upstream default (`false`) instead of our custom `true`: the server overrides it per-account at runtime, so the custom default was both divergent and unnecessary.
Verified: build, clippy --all-targets -D warnings, and the full test suite (excluding e2e) all pass.
📝 WalkthroughSummary by CodeRabbit
WalkthroughWacore props are migrated from numeric config codes to typed AbProp values. AbPropsCache API and WATCHED seed are updated, and all privacy-token gating (contacts, groups, send paths) now reads AbProp constants from ChangesPrivacy-Token Feature Flag Type Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labelsapi-design, breaking-change 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Benchmark Results67 unchanged benchmark(s)
|
![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: 8938f585d0
ℹ️ 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".
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wacore/src/store/ab_props.rs`:
- Around line 84-87: The is_enabled method currently calls is_enabled_or(prop,
false) which ignores the AbProp's typed default; change is_enabled(&self, prop:
AbProp) to call is_enabled_or(prop, prop.default().as_bool()) (or equivalent
retrieval of the AbProp default) so missing flags use AbProp.default; keep
is_enabled_or for explicit overrides. Update references in tests to add a
regression that asserts stale::PROFILE_PIC_PRIVACY_TOKEN (declared with
AbDefault::Bool(true)) returns true from is_enabled() when not set.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1b46432a-7370-4921-9a22-0219a0b4724d
📒 Files selected for processing (7)
src/features/contacts.rssrc/features/groups.rssrc/send.rswacore/src/iq/abprops.rswacore/src/iq/mod.rswacore/src/iq/props.rswacore/src/store/ab_props.rs
…from the registry Address the adversarial review of this PR. The typed `AbProp` already carries its upstream `default`, but the getters only read `.code`, so every callsite re-supplied the default by hand — two sources of truth that could silently diverge on a registry regen. `is_enabled(prop)` now falls back to `prop.default` (Bool), and `get_int(prop)` to `prop.default` (Int), dropping the explicit-default `is_enabled_or` and the 2-arg `get_int`. Callsites lose their literals: the `604800`/`4` tctoken magic numbers and the `false`/`true` flags now come from the registry. Behavior is unchanged — every removed literal already equaled the registry default (verified). Also refresh the stale `ab_props.rs` doc comments (`config_code` terminology and the outdated "~1,200 props" count). The reviewer's other note (getters take `AbProp` by value vs `&AbProp`) is declined: `AbProp` is `Copy` and a 48-byte copy on an async RwLock + HashMap path is negligible, and by-value reads cleaner than threading `&` through every callsite. Verified: build, clippy --all-targets -D warnings, full test suite (excluding e2e) pass.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/store/ab_props.rs (1)
133-174: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAdd a regression test for a default-true flag returning
truewhen missing.Look, the core
is_enabled()fix is solid now — it actually respectsAbProp.defaultlike it should. But we're still missing test coverage for the default-true scenario.stale::PROFILE_PIC_PRIVACY_TOKENhasAbDefault::Bool(true), and we should verifyis_enabled()returnstruewhen the server never sent that prop.Meta built its empire on test coverage. This is how we make sure nobody accidentally breaks this behavior again.
🧪 Suggested test addition
#[tokio::test] async fn is_enabled_honors_default_true_flag() { use crate::iq::props::stale; let cache = AbPropsCache::new(); // PROFILE_PIC_PRIVACY_TOKEN has default: AbDefault::Bool(true) // Server never sent it, so is_enabled must return true assert!(cache.is_enabled(stale::PROFILE_PIC_PRIVACY_TOKEN).await); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wacore/src/store/ab_props.rs` around lines 133 - 174, Add a regression test that constructs an AbPropsCache and asserts that is_enabled returns true for a flag whose AbDefault is Bool(true) when no prop was sent; specifically, create a #[tokio::test] that uses AbPropsCache::new() and calls cache.is_enabled(stale::PROFILE_PIC_PRIVACY_TOKEN).await to assert true, referencing AbPropsCache and is_enabled so the test verifies the default-true behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wacore/src/store/ab_props.rs`:
- Around line 133-174: Add a regression test that constructs an AbPropsCache and
asserts that is_enabled returns true for a flag whose AbDefault is Bool(true)
when no prop was sent; specifically, create a #[tokio::test] that uses
AbPropsCache::new() and calls
cache.is_enabled(stale::PROFILE_PIC_PRIVACY_TOKEN).await to assert true,
referencing AbPropsCache and is_enabled so the test verifies the default-true
behavior is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 446c53ba-a5c6-467f-b486-ae9d01c85523
📒 Files selected for processing (3)
src/features/contacts.rssrc/send.rswacore/src/store/ab_props.rs
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.
Replaces the hand-curated
config_codes(11 bareu32s) with the whatspec-generated typed A/B-props registry, vendored aswacore::iq::abprops. Three modules mirror WA Web's own split (web1775,hybrid312,group13 = 2100 flags); each flag is apub const AbProp { name, code, value_type, default }. Refresh by re-copying whatspec'sgenerated/abprops/abprops.rsand runningcargo fmt.Type-safety + usability
Cache getters (
get/is_enabled/is_enabled_or/get_int/watch/watch_many) now take a typedAbPropinstead of a magicu32. Callsites reference named flags with metadata:apply_propsstays keyed by the wireu32(the server sends codes).props::WATCHED(the 11 flags this client reads) seeds the cache interest set, replacingconfig_codes::ALL.Zero binary cost for unused flags
consts emit no symbol unless referenced, so the ~2089 unused flags cost nothing in the binary (this isconstsemantics, not even LTO-dependent). Confirmed nothing references the per-module or top-levelALLaggregates, so those 2100-entry arrays are never emitted either. Only the flags reachable fromWATCHED(and their name strings) materialize.Stale flags
Two flags the current WA Web bundle no longer ships (
privacy_token_only_check_lid15491,profile_pic_privacy_token9666) are absent from the registry. They live inprops::staleas explicit consts to preserve behavior: the server never sends them, so gating on them always falls to the callsite default (this was already the case). Worth revisiting whether those gated features still make sense, but out of scope here.LID default now matches upstream
LID_TRUSTED_TOKEN_ISSUE_TO_LIDpreviously used a customtruedefault; it now follows the upstream default (false). The server overrides A/B-props per-account at runtime, so the custom default was both divergent from WA Web and unnecessary.Verification
cargo build,cargo clippy --all-targets -- -D warnings, and the full test suite (excluding e2e) all pass. The drift cross-check confirmed the 9 live codes match the registry (the old curated names were stale:_SENDING_infix,WA_prefix).