perf: split notification handler, boxed-slice children, interest-based props - #553
Merged
Conversation
Extract each notification type's handling into a separate async fn (handle_encrypt_notification, handle_server_sync_notification, etc.) so the compiler doesn't size the dispatcher future for all 14 arms simultaneously. server_sync is now a sync fn (it only spawns a task). The per-task future size reduction benefits concurrent load scenarios where many notification tasks are alive simultaneously. DHAT total is unchanged (+0.9% noise) since the benchmark has low concurrency.
NodeContentRef::Nodes now stores Box<[NodeRef]> (boxed slice, 16 bytes) instead of Box<Vec<NodeRef>> (boxed Vec, 24 bytes + heap header). The decoder builds a Vec with exact capacity then converts to boxed slice via into_boxed_slice(). Saves 8 bytes per node with children and eliminates excess Vec capacity. DHAT: within noise (+0.8%), structural improvement.
Previously PropsResponse parsed all ~1,200 AB config properties into AbPropConfig enum structs, then AbPropsCache stored them all in a HashMap. No features currently query individual prop values. Now: props are parsed as lightweight (code, CompactString) tuples. AbPropsCache uses an interest set -- only watched config codes are retained, rest are discarded. watch()/watch_many() register codes. This eliminates ~982KB of parsing + ~811KB of HashMap inserts. The architecture is ready for future prop queries: call ab_props.watch(PropCode::FOO) before connect, then ab_props.is_enabled(PropCode::FOO) to read. Measured impact: - DHAT total: 28.1 MB -> 27.2 MB (-963 KB, -3.4%) - connect_to_ready bytes: 3.57 MB -> 3.40 MB (-166 KB, -4.6%)
📝 WalkthroughWalkthroughRefactors A/B props pipeline to consume experiment-only tuples and interest-based watches; updates client to pass delta flag and experiment props; replaces boxed Vec children with boxed slices in binary node types; and splits notification handling into dedicated helper functions preserving prior spawn-and-sync behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Notif as NotificationHandler
participant Backend as LocalBackendState
participant ConnGen as ConnectionGenerator
participant Sync as SyncTask
Notif->>Backend: parse collection nodes / check version
alt version supported
Notif->>ConnGen: request connection (cancellable)
ConnGen-->>Notif: connection or cancelled
Notif->>Sync: spawn sync task with parsed nodes
Sync->>Backend: perform sync operations
else unsupported
Notif--xBackend: skip sync (no spawn)
end
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)
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 |
jlucaso1
changed the title
Collaborator
Author
|
@claude review |
|
Claude finished @jlucaso1's task in 2m 39s —— View job Code Review: PR #553
Bug: Interest set is never populated — all AB props silently droppedSeverity: High
// wacore/src/store/ab_props.rs:67-70
for (code, value) in props {
if interest.contains(&code) { // interest is always empty!
map.insert(code, value);
}
}Meanwhile, the codebase actively queries ~12 different config codes via
All of these will now silently fall through to their defaults, which could change behavior around privacy token inclusion, tctoken timing, and group operations. Fix options:
Change 1: Notification handler split — Looks goodThe split of
No issues found. Change 2:
|
| Area | Verdict |
|---|---|
| Notification handler split | Approve — clean structural improvement |
Box<[NodeRef]> for children |
Approve — correct, saves 24 bytes per node list |
| Interest-based props filtering | Needs fix — interest set never populated, all props silently dropped |
The lightweight tuple parsing in PropsResponse is a solid optimization on its own. The interest-based filtering in AbPropsCache is a good idea architecturally but needs the interest set to actually be wired up before this can merge.
![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: 07ee4bf4b9
ℹ️ 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
+67
to
+69
| for (code, value) in props { | ||
| if interest.contains(&code) { | ||
| map.insert(code, value); |
There was a problem hiding this comment.
apply_props now filters every incoming prop through interest.contains(&code), but this commit does not register any production interest codes (watch/watch_many are only used in the test module). In the current runtime path (Client::fetch_props), full and delta responses therefore populate an empty cache, so all AB lookups fall back to defaults and experiment-driven behavior (including privacy/token config toggles) stops tracking server-provided values.
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: 4
🤖 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 1796-1798: The apply_props call currently seeds the cache only
with codes in interest so late watch() registrants never get an initial full
snapshot because is_seeded flips to true and future deltas are hashed; fix by
invalidating or clearing the seeded/hash state whenever interest grows (or when
a new watch is registered) so the next fetch will perform a full snapshot: add
logic in the watch registration path to compare previous interest size to new
interest and call a new/available method on ab_props (e.g.,
clear_seeded()/unset_seeded()/reset_hash_state) or modify apply_props to seed
with all known codes (not only watched) and ensure is_seeded is unset until full
snapshot covering new interest is stored. Ensure you reference and update
ab_props.apply_props, ab_props.is_seeded (or its setter/clear method), and the
watch()/interest growth code paths so late watchers receive the initial values.
In `@wacore/src/iq/props.rs`:
- Around line 507-510: The round-trip test is masking data loss because
PropsResponse::into_node() currently drops experiment_props; either update
PropsResponse::into_node() to include/serialize experiment_props into the Node
representation (so experiment_props survives round-trip), or change
test_props_response_protocol_node_round_trip to only assert metadata fields (and
not equality of experiment_props) to reflect that children are intentionally
omitted—locate and modify the PropsResponse::into_node() implementation to
include experiment_props serialization or adjust the test to scope assertions to
the preserved metadata fields accordingly.
- Around line 279-289: The loop that builds experiment_props currently swallows
malformed experiment <prop> entries; change it so that when a child has a
"config_code" attribute present but its value fails to parse into a positive u32
or its "config_value" is missing, the parser returns an error instead of
skipping the entry. Concretely, inside the for child in
node.get_children_by_tag("prop") block, use optional_attr(child, "config_code")
to detect presence, attempt parse and validate (>0), and if parse or validation
fails or optional_attr(child, "config_value") is None, return an Err (propagate
a descriptive parse error) rather than continuing; keep skipping props that lack
a config_code entirely (sampling props). This ensures Client::fetch_props won't
persist a new hash when experiment props are malformed.
In `@wacore/src/store/ab_props.rs`:
- Around line 62-71: The issue is that self.seeded.store(true,
Ordering::Release) is set before the full-update insert loop so readers may see
"seeded" while inserts are still in progress; in the non-delta branch (when
!delta_update) move the self.seeded.store(true, Ordering::Release) call to
immediately after the for (code, value) in props { ... } loop (i.e. clear the
map first as now, perform all map.insert(code, value) operations based on
interest, and only after the loop set self.seeded.store(true,
Ordering::Release)) so the seeded flag reflects completion of the full update.
🪄 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: 7c123b2a-8eca-44ac-971f-0f2dd240091f
📒 Files selected for processing (6)
src/client.rssrc/handlers/notification.rswacore/binary/src/decoder.rswacore/binary/src/node.rswacore/src/iq/props.rswacore/src/store/ab_props.rs
Comment on lines
+279
to
+289
| // Parse experiment props as lightweight (code, value) tuples. | ||
| // Sampling props (missing config_code or config_value) are skipped. | ||
| let mut experiment_props = Vec::new(); | ||
| for child in node.get_children_by_tag("prop") { | ||
| props.push(AbPropConfig::try_from_node_ref(child)?); | ||
| if let Some(code_str) = optional_attr(child, "config_code") | ||
| && let Ok(code) = code_str.parse::<u32>() | ||
| && code > 0 | ||
| && let Some(value) = optional_attr(child, "config_value") | ||
| { | ||
| experiment_props.push((code, CompactString::from(value.as_ref()))); | ||
| } |
There was a problem hiding this comment.
Don't treat malformed experiment props as ignorable noise.
This loop now silently drops any <prop> that advertises config_code but has an invalid code or missing config_value. Because Client::fetch_props persists the new hash after a successful parse, one malformed watched prop becomes a silent stale-cache entry with no retry path on later delta fetches. Keep skipping pure sampling props, but fail the response once a node claims to be an experiment prop and then fails validation.
Proposed fix
- for child in node.get_children_by_tag("prop") {
- if let Some(code_str) = optional_attr(child, "config_code")
- && let Ok(code) = code_str.parse::<u32>()
- && code > 0
- && let Some(value) = optional_attr(child, "config_value")
- {
- experiment_props.push((code, CompactString::from(value.as_ref())));
- }
- }
+ for child in node.get_children_by_tag("prop") {
+ let config_code = optional_attr(child, "config_code");
+ let event_code = optional_attr(child, "event_code");
+
+ if config_code.is_some() && event_code.is_some() {
+ return Err(anyhow::anyhow!(
+ "prop has both config_code and event_code (attrs: {:?})",
+ child.attrs
+ ));
+ }
+
+ if let Some(code_str) = config_code {
+ let code = code_str
+ .parse::<u32>()
+ .map_err(|e| anyhow::anyhow!("invalid config_code `{code_str}`: {e}"))?;
+ if code == 0 {
+ return Err(anyhow::anyhow!("config_code must be >= 1"));
+ }
+
+ let value = optional_attr(child, "config_value")
+ .ok_or_else(|| anyhow::anyhow!("missing config_value in prop"))?;
+ experiment_props.push((code, CompactString::from(value.as_ref())));
+ }
+ }📝 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
| // Parse experiment props as lightweight (code, value) tuples. | |
| // Sampling props (missing config_code or config_value) are skipped. | |
| let mut experiment_props = Vec::new(); | |
| for child in node.get_children_by_tag("prop") { | |
| props.push(AbPropConfig::try_from_node_ref(child)?); | |
| if let Some(code_str) = optional_attr(child, "config_code") | |
| && let Ok(code) = code_str.parse::<u32>() | |
| && code > 0 | |
| && let Some(value) = optional_attr(child, "config_value") | |
| { | |
| experiment_props.push((code, CompactString::from(value.as_ref()))); | |
| } | |
| // Parse experiment props as lightweight (code, value) tuples. | |
| // Sampling props (missing config_code or config_value) are skipped. | |
| let mut experiment_props = Vec::new(); | |
| for child in node.get_children_by_tag("prop") { | |
| let config_code = optional_attr(child, "config_code"); | |
| let event_code = optional_attr(child, "event_code"); | |
| if config_code.is_some() && event_code.is_some() { | |
| return Err(anyhow::anyhow!( | |
| "prop has both config_code and event_code (attrs: {:?})", | |
| child.attrs | |
| )); | |
| } | |
| if let Some(code_str) = config_code { | |
| let code = code_str | |
| .parse::<u32>() | |
| .map_err(|e| anyhow::anyhow!("invalid config_code `{code_str}`: {e}"))?; | |
| if code == 0 { | |
| return Err(anyhow::anyhow!("config_code must be >= 1")); | |
| } | |
| let value = optional_attr(child, "config_value") | |
| .ok_or_else(|| anyhow::anyhow!("missing config_value in prop"))?; | |
| experiment_props.push((code, CompactString::from(value.as_ref()))); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/iq/props.rs` around lines 279 - 289, The loop that builds
experiment_props currently swallows malformed experiment <prop> entries; change
it so that when a child has a "config_code" attribute present but its value
fails to parse into a positive u32 or its "config_value" is missing, the parser
returns an error instead of skipping the entry. Concretely, inside the for child
in node.get_children_by_tag("prop") block, use optional_attr(child,
"config_code") to detect presence, attempt parse and validate (>0), and if parse
or validation fails or optional_attr(child, "config_value") is None, return an
Err (propagate a descriptive parse error) rather than continuing; keep skipping
props that lack a config_code entirely (sampling props). This ensures
Client::fetch_props won't persist a new hash when experiment props are
malformed.
Comment on lines
+507
to
510
| experiment_props: vec![ | ||
| (100, CompactString::from("value1")), | ||
| (200, CompactString::from("value2")), | ||
| ], |
There was a problem hiding this comment.
This round-trip test now hides data loss.
PropsResponse::into_node() currently drops experiment_props, so removing the equality check means test_props_response_protocol_node_round_trip no longer exercises the field introduced by this PR. Either restore child serialization or rename/scope the test to metadata-only parsing; as written, the test passes over a lossy round trip.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/iq/props.rs` around lines 507 - 510, The round-trip test is
masking data loss because PropsResponse::into_node() currently drops
experiment_props; either update PropsResponse::into_node() to include/serialize
experiment_props into the Node representation (so experiment_props survives
round-trip), or change test_props_response_protocol_node_round_trip to only
assert metadata fields (and not equality of experiment_props) to reflect that
children are intentionally omitted—locate and modify the
PropsResponse::into_node() implementation to include experiment_props
serialization or adjust the test to scope assertions to the preserved metadata
fields accordingly.
Benchmark Results3 improvement(s):
56 unchanged benchmark(s)
|
The interest-based filtering was silently dropping all props because no production code called watch(). Pre-populate DEFAULT_INTEREST with all 11 config codes from config_codes module. Also: set seeded flag AFTER the insert loop (not before) so readers don't see seeded=true while inserts are in progress. Added regression test that verifies production config codes are retained without explicit watch() calls.
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/store/ab_props.rs`:
- Around line 59-64: In watch_many, replace the manual loop that inserts each
code into the interest write lock with a single extend call to express intent
more clearly: after acquiring let mut interest = self.interest.write().await,
call interest.extend(codes.iter().copied()) (or interest.extend(codes) if types
align) to add all codes at once; this keeps the method concise and idiomatic
while targeting the existing watch_many function and the interest collection.
🪄 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: 46a47fa9-5acc-4348-8986-ae959cd38d94
📒 Files selected for processing (1)
wacore/src/store/ab_props.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7b3d19de2
ℹ️ 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
259
to
261
| builder = builder.attr("delta_update", self.delta_update.to_string()); | ||
|
|
||
| let prop_nodes: Vec<Node> = self.props.into_iter().map(|p| p.into_node()).collect(); | ||
| builder = builder.children(prop_nodes); | ||
|
|
||
| builder.build() |
There was a problem hiding this comment.
PropsResponse::into_node now builds <props> with only attributes and never emits any <prop> children, so experiment_props is always dropped during serialization. Any caller that uses ProtocolNode::into_node for fixtures, mocks, or round-trip transformations will produce an empty props payload and parse back an empty experiment_props, which silently changes behavior from the pre-change contract.
Useful? React with 👍 / 👎.
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/store/ab_props.rs`:
- Around line 18-32: DEFAULT_INTEREST currently must be manually kept in sync
with config_codes in iq/props.rs; add a short cross-reference comment above
DEFAULT_INTEREST mentioning config_codes::(production list) and create a
compile-time/test-time assertion to catch drift: extend or add a unit test
(e.g., default_interest_retains_production_config_codes) to compare
DEFAULT_INTEREST.len() against the canonical count from config_codes (expose or
add a const like CONFIG_PRODUCTION_COUNT in that module or compute from a
central list) so the build/test fails if new production config codes are added
but not appended to DEFAULT_INTEREST.
🪄 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: ec34b721-034e-4303-a815-45b5fe9ee9e7
📒 Files selected for processing (1)
wacore/src/store/ab_props.rs
Replace duplicated DEFAULT_INTEREST list with config_codes::ALL. Adding a new config code now only requires updating one place. Also documents into_node() asymmetry (metadata only, no prop children).
There was a problem hiding this comment.
♻️ Duplicate comments (2)
wacore/src/iq/props.rs (2)
519-540: 🧹 Nitpick | 🔵 TrivialMake the lossy serialization contract explicit in this test.
PropsResponse::into_node()now dropsexperiment_props, so this is no longer a true round-trip test. Rename it to metadata-only round-trip or assert thatparsed.experiment_propsis empty so the data loss is intentional rather than implicit.Suggested tweak
- fn test_props_response_protocol_node_round_trip() { + fn test_props_response_metadata_round_trip() { let response = PropsResponse { ab_key: Some("test_ab_key".to_string()), hash: Some("hash123".to_string()), refresh: Some(7200), refresh_id: Some(42), @@ assert_eq!(parsed.hash, response.hash); assert_eq!(parsed.refresh, response.refresh); assert_eq!(parsed.refresh_id, response.refresh_id); assert_eq!(parsed.delta_update, response.delta_update); + assert!(parsed.experiment_props.is_empty()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/iq/props.rs` around lines 519 - 540, The test test_props_response_protocol_node_round_trip currently relies on PropsResponse::into_node which intentionally drops experiment_props, so make the lossy serialization explicit by either renaming the test to something like test_props_response_protocol_node_metadata_round_trip or by keeping the name and adding an assertion that parsed.experiment_props is empty; locate the test function and update its name and/or add assert!(parsed.experiment_props.is_empty()) after the other assertions to document the intentional data loss from PropsResponse::into_node and PropsResponse::try_from_node.
301-308:⚠️ Potential issue | 🟠 MajorReturn an error once a
<prop>claims to be an experiment prop but fails validation.Right now a delta response that contains
config_codebut has an invalid code, a missingconfig_value, or conflicting experiment/sampling attrs is treated as a successful parse and the entry is just dropped.AbPropsCache::apply_props(true, ...)then keeps the previous watched value as if nothing changed, which can leave the client on stale config after a malformed update.Proposed fix
let mut experiment_props = Vec::new(); for child in node.get_children_by_tag("prop") { - if let Some(code_str) = optional_attr(child, "config_code") - && let Ok(code) = code_str.parse::<u32>() - && code > 0 - && let Some(value) = optional_attr(child, "config_value") - { - experiment_props.push((code, CompactString::from(value.as_ref()))); + let config_code = optional_attr(child, "config_code"); + let event_code = optional_attr(child, "event_code"); + + if config_code.is_some() && event_code.is_some() { + return Err(anyhow::anyhow!( + "prop has both config_code and event_code (attrs: {:?})", + child.attrs + )); + } + + if let Some(code_str) = config_code { + let code = code_str + .parse::<u32>() + .map_err(|e| anyhow::anyhow!("invalid config_code `{code_str}`: {e}"))?; + if code == 0 { + return Err(anyhow::anyhow!("config_code must be >= 1")); + } + + let value = optional_attr(child, "config_value") + .ok_or_else(|| anyhow::anyhow!("missing config_value in prop"))?; + experiment_props.push((code, CompactString::from(value.as_ref()))); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/src/iq/props.rs` around lines 301 - 308, The loop that collects experiment_props currently ignores malformed <prop> entries (using node.get_children_by_tag("prop"), optional_attr, and pushing into experiment_props), causing silent drops; update that logic so that whenever optional_attr(child, "config_code") is Some but parsing fails, parsed code is zero, config_value is missing, or there are conflicting experiment/sampling attributes, the function returns an Err immediately (propagating a descriptive error) instead of skipping the entry—ensure you change the branch around the code_str.parse::<u32>() check and the value presence check to return an error on any validation failure so callers like AbPropsCache::apply_props(true, ...) can detect malformed updates.
🤖 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/iq/props.rs`:
- Around line 519-540: The test test_props_response_protocol_node_round_trip
currently relies on PropsResponse::into_node which intentionally drops
experiment_props, so make the lossy serialization explicit by either renaming
the test to something like test_props_response_protocol_node_metadata_round_trip
or by keeping the name and adding an assertion that parsed.experiment_props is
empty; locate the test function and update its name and/or add
assert!(parsed.experiment_props.is_empty()) after the other assertions to
document the intentional data loss from PropsResponse::into_node and
PropsResponse::try_from_node.
- Around line 301-308: The loop that collects experiment_props currently ignores
malformed <prop> entries (using node.get_children_by_tag("prop"), optional_attr,
and pushing into experiment_props), causing silent drops; update that logic so
that whenever optional_attr(child, "config_code") is Some but parsing fails,
parsed code is zero, config_value is missing, or there are conflicting
experiment/sampling attributes, the function returns an Err immediately
(propagating a descriptive error) instead of skipping the entry—ensure you
change the branch around the code_str.parse::<u32>() check and the value
presence check to return an error on any validation failure so callers like
AbPropsCache::apply_props(true, ...) can detect malformed updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f2baebe-4374-4973-8b92-4595eedcce55
📒 Files selected for processing (2)
wacore/src/iq/props.rswacore/src/store/ab_props.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2baf901514
ℹ️ 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
+260
to
+262
| /// Serializes metadata attrs only. Individual `<prop>` children are not | ||
| /// emitted since experiment_props stores lightweight (code, value) tuples | ||
| /// without the full AbPropConfig structure needed for node construction. |
There was a problem hiding this comment.
PropsResponse::try_from_node_ref still reads <prop> children into experiment_props, but into_node now intentionally emits only metadata attributes and never writes those children back. Any path that round-trips a response through into_node (e.g., protocol fixtures/mocks or transform pipelines) will silently lose all experiment values and then parse back as an empty prop set, which is a behavioral regression from the previous contract.
Useful? React with 👍 / 👎.
5 tasks
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
Three optimizations targeting handler futures, decoder layout, and props parsing.
Full optimization journey (original baseline -> now, all PRs combined):
This PR alone:
Changes
1. Split notification handler into per-type async functions
2. Box<[NodeRef]> for child nodes
3. Interest-based AB props filtering (-963 KB)
Test plan
Summary by CodeRabbit
Refactor
New Features