Skip to content

Conversation

@ayushjain17
Copy link
Collaborator

@ayushjain17 ayushjain17 commented Jan 15, 2026

Problem

Describe the problem you are trying to solve here

Solution

Provide a brief summary of your solution so that reviewers can understand your code

Environment variable changes

What ENVs need to be added or changed

Pre-deployment activity

Things needed to be done before deploying this change (if any)

Post-deployment activity

Things needed to be done after deploying this change (if any)

API changes

Endpoint Method Request body Response Body
API GET/POST, etc request response

Possible Issues in the future

Describe any possible issues that could occur because of this change

Summary by CodeRabbit

Release Notes

  • Refactor
    • Restructured internal condition representation from expression enums to direct value types.
    • Simplified context handling and construction patterns across the application.
    • Updated condition input components and callback signatures for improved data flow.
    • Refined condition display logic, particularly for dimension-based filtering.

✏️ Tip: You can customize this high-level summary in your review settings.

@ayushjain17 ayushjain17 requested a review from a team as a code owner January 15, 2026 20:52
@semanticdiff-com
Copy link

semanticdiff-com bot commented Jan 15, 2026

Review changes with  SemanticDiff

Changed Files
File Status
  crates/frontend/src/pages/experiment_groups.rs  81% smaller
  crates/frontend/src/pages/experiment.rs  75% smaller
  crates/frontend/src/pages/experiment_list/utils.rs  73% smaller
  crates/frontend/src/pages/experiment_list.rs  67% smaller
  crates/frontend/src/components/cohort_schema.rs  66% smaller
  crates/frontend/src/pages/experiment_group_listing.rs  62% smaller
  crates/frontend/src/components/experiment.rs  47% smaller
  crates/frontend/src/components/context_card.rs  46% smaller
  crates/frontend/src/pages/context_override/filter.rs  42% smaller
  crates/frontend/src/pages/experiment_list/filter.rs  41% smaller
  crates/frontend/src/pages/context_override.rs  41% smaller
  crates/frontend/src/components/context_form.rs  40% smaller
  crates/frontend/src/components/condition_pills.rs  39% smaller
  crates/frontend/src/components/experiment_form/utils.rs  36% smaller
  crates/frontend/src/logic.rs  18% smaller
  crates/frontend/src/components/input.rs  17% smaller
  crates/frontend/src/pages/home.rs  13% smaller
  crates/frontend/src/pages/context_override/utils.rs  5% smaller
  crates/frontend/src/api.rs  5% smaller
  crates/frontend/src/components/variant_form.rs  4% smaller
  crates/frontend/src/pages/compare_overrides.rs  2% smaller
  crates/frontend/src/components/experiment_form.rs  0% smaller
  crates/frontend/src/components/experiment_group_form.rs  0% smaller
  crates/superposition_types/src/config.rs  0% smaller
  crates/superposition_types/src/custom_query.rs  0% smaller

@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This PR refactors condition and context representation throughout the frontend by removing Expression and Operator enums in favor of direct serde_json::Value usage. Multiple components and pages are updated to use new conversion paths (from_iter, Map::from, into()) replacing prior JSON serialization methods. Public API changes include removing on_operator_change callbacks and restructuring helper functions.

Changes

Cohort / File(s) Summary
Core Type System Changes
crates/superposition_types/src/config.rs, crates/superposition_types/src/custom_query.rs, crates/frontend/src/logic.rs
Removed Expression and Operator enums; replaced with direct Value usage. Condition.expression field → Condition.value. Added FromIterator<(String, Value)> for Conditions and From for Map. New public methods: Condition::into_inner() and IntoIterator impl for QueryMap. Refactored all context-to-Condition conversion paths.
Component Signature Updates
crates/frontend/src/components/condition_pills.rs, crates/frontend/src/components/context_form.rs
condition_pills: Added resolve_summary: bool parameter to condition_expression() and condition() functions. context_form: Removed on_operator_change callback; changed on_remove to Callback<()> and on_value_change to Callback. Introduced private ResolveOperator enum.
Component Context/Conversion Updates
crates/frontend/src/components/context_card.rs, crates/frontend/src/components/cohort_schema.rs, crates/frontend/src/components/api.rs, crates/frontend/src/components/experiment_form.rs, crates/frontend/src/components/experiment_group_form.rs, crates/frontend/src/components/variant_form.rs
Updated context conversions: replaced as_context_json() with into(), as_resolve_context() with Map::from(), and from_context_json() with from_iter(). Updated FunctionEnvironment context field initialization across all files.
Input and Form Updates
crates/frontend/src/components/input.rs, crates/frontend/src/components/experiment_form/utils.rs
array_input: Added value_rws RwSignal for separate input tracking; refactored Enter key handling with duplicate detection and parsing validation. experiment_form/utils.rs: Updated context field construction to use Map::from(conditions) with Exp::::try_from().
Page Component Updates
crates/frontend/src/pages/experiment.rs, crates/frontend/src/pages/home.rs, crates/frontend/src/pages/compare_overrides.rs, crates/frontend/src/pages/experiment_group_listing.rs, crates/frontend/src/pages/experiment_groups.rs, crates/frontend/src/pages/experiment_list.rs
Removed import aliases (ConditionComponent → Condition). Replaced Conditions::from_context_json() with from_iter(). Replaced context conversions (as_context_json() → into(), as_resolve_context() → Map::from()). Removed filtering of "variantIds" from dimensions. Updated all Condition component usages.
Page Context/Filter Utils
crates/frontend/src/pages/context_override.rs, crates/frontend/src/pages/context_override/utils.rs, crates/frontend/src/pages/context_override/filter.rs, crates/frontend/src/pages/experiment_list/utils.rs, crates/frontend/src/pages/experiment_list/filter.rs
Standardized context handling: replaced JSON-based conversions with from_iter() pattern. Updated FunctionEnvironment initialization (as_context_json() → into()). Replaced DimensionQuery construction (as_resolve_context() → Map::from()). Added resolve_summary=true to Condition components. Removed manual Condition/Expression mapping logic.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Suggested reviewers

  • knutties
  • mahatoankitkumar

Poem

🐰 Expressions transformed to Values bright,
Context flows through iterators' light,
From JSON paths to Maps we leap,
Callbacks simplified, conversions deep,
A refactor's hop through frontend's might! 🌟

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 51.85% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: enabling variant IDs as arrays for resolve/filter functionality in the UI, which is reflected throughout the codebase changes involving resolve_summary flags and array handling.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@crates/frontend/src/components/input.rs`:
- Around line 609-611: The debug logging statement inside the render closure is
leaking user-entered values by calling logging::log!("val: {}", value_rws.get())
on every render; remove that logging call from the move || { ... } closure (the
closure that returns the view! block) so user input is no longer emitted to the
console, and ensure no other logging of value_rws.get() remains in the same
render path (search for logging::log! usages near this closure and in the Input
component).
- Around line 612-635: The input currently uses key=value_rws.get() causing
remounts and contains a debug logging call; remove the key=... prop from the
<input> to avoid forced remounting, delete any logging::log!() calls that print
keystrokes, and convert to a controlled-input pattern: keep a local
mutable/string state (the existing value_rws or a new temp state) bound to the
input value, on Enter parse into T (using the same parse/enqueue_alert logic in
the on:keydown handler) and on successful add push to options_rws, call
on_change with options_rws.get(), and then clear the controlled input state so
the field empties without remounting; ensure unique handling (unique flag and
options_rws.contains) stays the same.
🧹 Nitpick comments (3)
crates/frontend/src/components/variant_form.rs (1)

641-645: Avoid borrowing a temporary map across the async call.

&context.clone().into() creates a temporary map that’s immediately borrowed; factoring it into a local improves clarity and avoids repeated conversions (use the same snapshot for both get_context_from_condition and DimensionQuery).

♻️ Suggested refactor
-            let context_data = match context_data {
+            let context_map: Map<String, Value> = context.clone().into();
+            let context_data = match context_data {
                 Some(data) => Ok(data),
                 None => get_context_from_condition(
-                    &context.clone().into(),
+                    &context_map,
                     &workspace,
                     &org_id,
                 )
                 .await
                 .map(|context| (context.id, context.override_.into())),
             };
             match context_data {
                 Ok((context_id, overrides)) => {
-                    let context = DimensionQuery::from(Map::from(context));
+                    let context = DimensionQuery::from(context_map.clone());
crates/frontend/src/components/condition_pills.rs (1)

65-71: Consider extracting operator logic to a helper function.

The inline conditional for determining the operator works correctly but could benefit from extraction for clarity and testability.

♻️ Optional refactor
+fn get_operator(variable: &str, resolve_summary: bool) -> &'static str {
+    if variable == "variantIds" {
+        if resolve_summary { "in" } else { "has" }
+    } else {
+        "=="
+    }
+}
+
 // Then in the component:
 let (dimension, operator, operands) = condition
     .with_value(|v| {
         (
             v.variable.clone(),
-            if v.variable == "variantIds" && resolve_summary {
-                "in"
-            } else if v.variable == "variantIds" {
-                "has"
-            } else {
-                "=="
-            },
+            get_operator(&v.variable, resolve_summary),
             v.value.html_display(),
         )
     });
crates/frontend/src/components/context_form.rs (1)

121-140: Consider handling empty/malformed array values more defensively.

The unwrap_or(&vec![]) on line 127 handles the case where the value isn't an array, but silent fallback to an empty vector might hide data issues.

💡 Optional: Add logging for unexpected value types
                     ResolveOperator::In => {
+                        if condition.value.as_array().is_none() && !condition.value.is_null() {
+                            logging::warn!("Expected array value for variantIds, got: {:?}", condition.value);
+                        }
                         view! {
                             <StringArrayInput
                                 options=condition
                                     .value
                                     .as_array()
                                     .unwrap_or(&vec![])
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73fe5af and 8567261.

📒 Files selected for processing (25)
  • crates/frontend/src/api.rs
  • crates/frontend/src/components/cohort_schema.rs
  • crates/frontend/src/components/condition_pills.rs
  • crates/frontend/src/components/context_card.rs
  • crates/frontend/src/components/context_form.rs
  • crates/frontend/src/components/experiment.rs
  • crates/frontend/src/components/experiment_form.rs
  • crates/frontend/src/components/experiment_form/utils.rs
  • crates/frontend/src/components/experiment_group_form.rs
  • crates/frontend/src/components/input.rs
  • crates/frontend/src/components/variant_form.rs
  • crates/frontend/src/logic.rs
  • crates/frontend/src/pages/compare_overrides.rs
  • crates/frontend/src/pages/context_override.rs
  • crates/frontend/src/pages/context_override/filter.rs
  • crates/frontend/src/pages/context_override/utils.rs
  • crates/frontend/src/pages/experiment.rs
  • crates/frontend/src/pages/experiment_group_listing.rs
  • crates/frontend/src/pages/experiment_groups.rs
  • crates/frontend/src/pages/experiment_list.rs
  • crates/frontend/src/pages/experiment_list/filter.rs
  • crates/frontend/src/pages/experiment_list/utils.rs
  • crates/frontend/src/pages/home.rs
  • crates/superposition_types/src/config.rs
  • crates/superposition_types/src/custom_query.rs
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ayushjain17
Repo: juspay/superposition PR: 827
File: crates/superposition_core/src/config.rs:154-154
Timestamp: 2026-01-07T20:38:59.468Z
Learning: The superposition codebase has deprecated jsonlogic-based context conditions in favor of simple key-value pair map conditions. The `superposition_types::apply` function is now used for condition evaluation instead of `jsonlogic::apply`, and this represents an intentional architectural change across the entire system.
📚 Learning: 2026-01-07T20:38:53.153Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 827
File: crates/superposition_core/src/config.rs:154-154
Timestamp: 2026-01-07T20:38:53.153Z
Learning: Deprecate jsonlogic-based context conditions and migrate to simple key-value pair map conditions across the codebase. Replace jsonlogic::apply usage with superposition_types::apply for condition evaluation. Update all relevant modules (notably Rust files under crates) to use the new approach, remove jsonlogic dependencies where applicable, and run full compilation and tests to catch regressions. Ensure context evaluation logic consistently uses key-value maps and that the architectural change is reflected in unit/integration tests.

Applied to files:

  • crates/superposition_types/src/config.rs
  • crates/frontend/src/components/experiment_group_form.rs
  • crates/frontend/src/pages/context_override/utils.rs
  • crates/frontend/src/components/experiment_form.rs
  • crates/frontend/src/components/experiment.rs
  • crates/frontend/src/pages/experiment_list.rs
  • crates/frontend/src/components/variant_form.rs
  • crates/frontend/src/pages/home.rs
  • crates/superposition_types/src/custom_query.rs
  • crates/frontend/src/pages/experiment_groups.rs
  • crates/frontend/src/components/cohort_schema.rs
  • crates/frontend/src/pages/experiment_group_listing.rs
  • crates/frontend/src/pages/context_override/filter.rs
  • crates/frontend/src/pages/compare_overrides.rs
  • crates/frontend/src/pages/experiment_list/filter.rs
  • crates/frontend/src/components/context_card.rs
  • crates/frontend/src/components/experiment_form/utils.rs
  • crates/frontend/src/pages/context_override.rs
  • crates/frontend/src/components/condition_pills.rs
  • crates/frontend/src/api.rs
  • crates/frontend/src/pages/experiment.rs
  • crates/frontend/src/components/input.rs
  • crates/frontend/src/logic.rs
  • crates/frontend/src/pages/experiment_list/utils.rs
  • crates/frontend/src/components/context_form.rs
📚 Learning: 2026-01-05T12:37:05.828Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 815
File: crates/frontend/src/components/datetime.rs:70-95
Timestamp: 2026-01-05T12:37:05.828Z
Learning: Documentation should reflect build-target dependent behavior. For functions that differ between wasm32 and non-wasm32 (e.g., datetime parsing/normalization), explicitly document the intent and the platform-specific logic in the code and/or a nearby comment. Prefer using cfg attributes to clearly separate implementations for wasm32 vs non-wasm32 and add tests that cover both builds. In this case, the function should be noted as: wasm32 -> local midnight interpreted then converted to UTC; non-wasm32 (SSR) -> UTC midnight directly. Ensure any related behavior is consistent with the SSR/CSR architecture and that downstream code relies on the clarified semantics.

Applied to files:

  • crates/frontend/src/components/experiment_group_form.rs
  • crates/frontend/src/components/experiment_form.rs
  • crates/frontend/src/components/experiment.rs
  • crates/frontend/src/components/variant_form.rs
  • crates/frontend/src/components/cohort_schema.rs
  • crates/frontend/src/components/context_card.rs
  • crates/frontend/src/components/experiment_form/utils.rs
  • crates/frontend/src/components/condition_pills.rs
  • crates/frontend/src/components/input.rs
  • crates/frontend/src/components/context_form.rs
📚 Learning: 2026-01-03T10:25:41.325Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 814
File: crates/superposition_types/src/api/workspace.rs:70-77
Timestamp: 2026-01-03T10:25:41.325Z
Learning: The `auto_populate_control` field in `CreateWorkspaceRequest` (`crates/superposition_types/src/api/workspace.rs`) is expected to default to `true`, not `false`. This is implemented using `#[serde(default = "default_true")]`.

Applied to files:

  • crates/frontend/src/components/variant_form.rs
📚 Learning: 2026-01-03T13:25:40.584Z
Learnt from: ayushjain17
Repo: juspay/superposition PR: 816
File: crates/frontend/src/pages/webhook.rs:136-137
Timestamp: 2026-01-03T13:25:40.584Z
Learning: In the superposition codebase (Rust frontend), the `Workspace` and `OrganisationId` newtype wrappers implement `Deref`, which allows `&Workspace` and `&OrganisationId` to be automatically coerced to `&str` when passed to functions expecting `&str` parameters. Manual `.0` dereferencing is not needed.

Applied to files:

  • crates/frontend/src/components/context_form.rs
🧬 Code graph analysis (16)
crates/superposition_types/src/config.rs (3)
crates/superposition_types/src/custom_query.rs (3)
  • into_inner (84-84)
  • into_inner (105-107)
  • into_inner (153-155)
crates/superposition_types/src/lib.rs (2)
  • into_inner (97-99)
  • into_inner (105-107)
crates/superposition_types/src/database/models/experimentation.rs (1)
  • into_inner (270-272)
crates/frontend/src/components/experiment_group_form.rs (1)
crates/frontend/src/components/context_form.rs (1)
  • context_rs (282-287)
crates/frontend/src/pages/context_override/utils.rs (1)
crates/frontend/src/logic.rs (1)
  • from (74-76)
crates/frontend/src/components/experiment_form.rs (3)
crates/superposition_sdk/src/operation/get_resolved_config/_get_resolved_config_input.rs (1)
  • context (204-209)
crates/superposition_sdk/src/operation/get_resolved_config/builders.rs (1)
  • context (222-225)
crates/frontend/src/components/context_form.rs (1)
  • context_rs (282-287)
crates/frontend/src/components/experiment.rs (1)
crates/frontend/src/logic.rs (2)
  • from_iter (58-60)
  • from_iter (64-70)
crates/frontend/src/components/variant_form.rs (2)
crates/superposition_types/src/custom_query.rs (3)
  • from (205-207)
  • from (211-218)
  • from (235-237)
crates/superposition_types/src/lib.rs (4)
  • from (280-285)
  • from (300-302)
  • from (306-308)
  • from (312-314)
crates/frontend/src/pages/home.rs (3)
crates/frontend/src/logic.rs (5)
  • iter (59-59)
  • iter (66-68)
  • from_iter (58-60)
  • from_iter (64-70)
  • from (74-76)
crates/frontend/src/types.rs (4)
  • from_iter (58-60)
  • from_iter (97-99)
  • from (84-90)
  • from (260-266)
crates/frontend/src/components/context_form.rs (2)
  • context_rs (282-287)
  • from (35-40)
crates/superposition_types/src/custom_query.rs (1)
crates/superposition_types/src/config.rs (1)
  • into_iter (92-94)
crates/frontend/src/components/cohort_schema.rs (1)
crates/frontend/src/logic.rs (2)
  • to_condition_json (122-124)
  • to_condition_json (169-176)
crates/frontend/src/pages/experiment_group_listing.rs (2)
crates/frontend/src/logic.rs (2)
  • from_iter (58-60)
  • from_iter (64-70)
crates/frontend/src/types.rs (2)
  • from_iter (58-60)
  • from_iter (97-99)
crates/frontend/src/pages/context_override/filter.rs (1)
crates/frontend/src/logic.rs (3)
  • from_iter (58-60)
  • from_iter (64-70)
  • from (74-76)
crates/frontend/src/pages/experiment_list/filter.rs (1)
crates/frontend/src/logic.rs (3)
  • from_iter (58-60)
  • from_iter (64-70)
  • from (74-76)
crates/frontend/src/components/context_card.rs (1)
crates/frontend/src/logic.rs (2)
  • from_iter (58-60)
  • from_iter (64-70)
crates/frontend/src/pages/context_override.rs (5)
crates/frontend/src/components/context_form.rs (1)
  • context_rs (282-287)
crates/superposition_sdk/src/operation/get_resolved_config/_get_resolved_config_input.rs (1)
  • context (204-209)
crates/superposition_sdk/src/operation/get_resolved_config/builders.rs (1)
  • context (222-225)
crates/frontend/src/logic.rs (2)
  • from_iter (58-60)
  • from_iter (64-70)
crates/frontend/src/types.rs (2)
  • from_iter (58-60)
  • from_iter (97-99)
crates/frontend/src/logic.rs (1)
crates/frontend/src/schema.rs (4)
  • html_display (7-7)
  • html_display (11-29)
  • html_display (33-35)
  • from (60-69)
crates/frontend/src/pages/experiment_list/utils.rs (2)
crates/frontend/src/logic.rs (2)
  • from_iter (58-60)
  • from_iter (64-70)
crates/frontend/src/types.rs (2)
  • from_iter (58-60)
  • from_iter (97-99)
🔇 Additional comments (52)
crates/frontend/src/components/variant_form.rs (1)

98-98: No changes needed; this conversion correctly preserves array types.

The From<Conditions> for Map<String, Value> implementation directly extracts c.value from each condition without stringification or flattening. Since Condition::value is already serde_json::Value, arrays remain as Value::Array in the resulting map. The conversion is simple and correct:

impl From<Conditions> for Map<String, Value> {
    fn from(value: Conditions) -> Self {
        value.0.into_iter().map(|c| (c.variable, c.value)).collect()
    }
}

No regression test is required because the conversion logic is straightforward and poses no risk to variant-id arrays.

Likely an incorrect or invalid review comment.

crates/frontend/src/components/experiment_form/utils.rs (2)

1-1: Import update looks correct.

Adds Map needed for the new context construction.


47-47: Map-based context conversion is consistent.

The new Map::from(conditions) path matches the PR’s shift toward map/iterator-based contexts.

crates/frontend/src/components/experiment_group_form.rs (1)

194-194: LGTM for direct context conversion.

The into() conversion fits the new map-based context flow.

crates/superposition_types/src/config.rs (1)

183-185: Good addition for ergonomics.

into_inner mirrors existing wrapper patterns and enables map-based conversions cleanly.

crates/frontend/src/components/context_card.rs (1)

95-95: Iterator-based Conditions build looks good.

This aligns the UI path with the new map-backed Condition model.

crates/superposition_types/src/custom_query.rs (1)

195-201: Clean iterator impl.

Delegating to the inner Map keeps iteration behavior consistent.

crates/frontend/src/logic.rs (4)

7-18: Value-based Condition defaults look good.

The value field and default initialization align with the new Value-centric model.


45-76: Condition collection conversions look clean.

FromIterator and From<Conditions> for Map<String, Value> make the Map-based context flow straightforward.


85-176: Value-centric jsonlogic helpers are consistent.

try_from_condition_map and to_condition_json now operate directly on Value, which matches the updated Condition model.


20-24: The to_condition_query_str method appears to be unused and newly added code.

The method was introduced in commit 8567261 ("feat(UI): allow variant ids as array for resolve/filter") but has no call sites in the codebase. The design to return None for "variantIds" is intentional and correct: variantIds is managed separately by the backend experimentation system and inserted directly into query_data (not encoded as query string parameters). This is evident from backend handlers in crates/superposition/src/resolve/handlers.rs and crates/superposition_core/src/ffi.rs which explicitly insert variantIds via query_data.insert("variantIds".to_string(), variants.into()).

The frontend already implements special handling for variantIds: filtering it from dimension lists (context_form.rs), displaying it with special operators (condition_pills.rs), and preventing actions when present (context_card.rs). Confirm whether this method will be integrated into query string construction, and if so, ensure callers handle the None return properly rather than dropping the entire condition.

crates/frontend/src/api.rs (1)

1082-1090: Context conversion in experiment-group create matches the new Conditions API.

Using Map::from(conditions) aligns with the Value-based context representation.

crates/frontend/src/components/cohort_schema.rs (1)

532-571: ConditionInput wiring matches the new Value-based helpers.

Conditions(...).into() and to_condition_json(...) are consistent with the updated logic flow.

crates/frontend/src/components/experiment_form.rs (1)

134-143: FunctionEnvironment context via Into looks good.

This aligns the form with the new Conditions → Map conversion path.

crates/frontend/src/pages/experiment.rs (2)

84-84: LGTM! Simplified dimension handling.

Removing the filter for "variantIds" aligns with the PR objective to allow variant IDs as an array. All dimensions are now passed through directly.


151-153: LGTM! Context construction migrated to iterator-based approach.

The change from JSON-based context parsing to Conditions::from_iter(experiment_ef.context.into_inner()) aligns with the codebase-wide migration from jsonlogic-based conditions to simple key-value pair maps. Based on learnings, this is an intentional architectural change.

crates/frontend/src/components/experiment.rs (2)

313-314: LGTM! Context conversion aligned with new architecture.

The context construction via Conditions::from_iter(v.context.clone().into_inner()) correctly transforms the experiment context into the new Conditions representation. The clone() is appropriate here since with_value provides a reference.


374-374: LGTM! Value display updated.

Displaying condition.value.html_display() instead of the previous expression-based approach aligns with the migration to Value-centric condition handling.

crates/frontend/src/components/condition_pills.rs (1)

35-35: LGTM! New resolve_summary parameter added for variant ID handling.

The resolve_summary boolean flag is properly threaded through the component hierarchy, enabling differentiated display of "variantIds" conditions based on context (resolve/filter vs. normal display).

Also applies to: 100-100

crates/frontend/src/pages/context_override/utils.rs (1)

21-21: LGTM! Context payload construction simplified.

Using Map::from(conditions) leverages the From<Conditions> implementation (as seen in logic.rs lines 73-75) to convert conditions to a Map<String, Value>. This is cleaner than the previous as_context_json() approach and aligns with the codebase-wide migration to Map-based context representation.

crates/frontend/src/pages/context_override.rs (3)

112-112: LGTM! FunctionEnvironment context serialization updated.

The .into() conversion replaces the previous as_context_json() call, using the trait implementation to convert Conditions to the appropriate map type for FunctionEnvironment.


253-256: LGTM! Context data loading migrated to iterator-based construction.

The Conditions::from_iter(context.value.clone().into_inner()) pattern correctly transforms the stored context value into a Conditions instance using the FromIterator implementation for (String, Value) pairs.


448-448: LGTM! Simplified dimension handling consistent with other files.

Removing dimension filtering aligns with the changes in experiment.rs, ensuring all dimensions (including "variantIds") are available for context forms.

crates/frontend/src/pages/context_override/filter.rs (4)

114-116: LGTM! Clean migration to iterator-based Conditions construction.

The change from manual context construction to Conditions::from_iter(dimension_params.clone().into_inner()) aligns with the codebase-wide migration to simple key-value pair map conditions. Based on learnings, this is consistent with the deprecation of jsonlogic-based conditions.


234-239: LGTM! Context initialization and FunctionEnvironment construction updated correctly.

The context signal initialization using Conditions::from_iter and the into() conversion for FunctionEnvironment.context are consistent with the broader refactor pattern seen across the codebase.


256-259: LGTM! DimensionQuery construction updated to use Map-based conversion.

The change to DimensionQuery::from(Map::from(context)) follows the new pattern for context handling, replacing the previous as_resolve_context() approach.


129-129: The resolve_summary=true prop is correctly handled by the Condition component. It properly controls the operator selection for variantIds (changing from "has" to "in"), enabling the intended resolve mode behavior.

crates/frontend/src/components/context_form.rs (5)

29-50: LGTM! Clean implementation of ResolveOperator for UI rendering.

The ResolveOperator enum with From<&str> and Display implementations provides a clean separation for handling the "variantIds" dimension differently from other dimensions. The pattern matching on dimension name is straightforward.


60-61: Good simplification of the callback signatures.

Removing on_operator_change and simplifying on_remove to Callback<()> and on_value_change to Callback<Value> makes the API cleaner since the operator is now derived from the dimension name.


191-198: LGTM! Conditional dimension filtering based on resolve_mode.

The logic correctly includes "variantIds" dimension only when resolve_mode is true, which aligns with the PR objective of allowing variant IDs as array for resolve/filter operations.


246-258: LGTM! Default value handling for variantIds dimension.

The special case for variantIds in resolve_mode to use Value::Array(vec![]) as the default value is correct, as it expects an array of variant IDs rather than a scalar value derived from the schema.


329-341: LGTM! Simplified callback implementations.

The on_value_change now passes a tuple (idx, Condition) which allows the parent to update the correct condition in the context. The on_remove is simplified to just pass the index.

crates/frontend/src/pages/home.rs (3)

90-97: LGTM! Updated context iteration and Conditions construction.

The change to into_iter() for ownership and Conditions::from_iter(context.condition.into_inner()) for constructing conditions aligns with the codebase-wide migration to iterator-based construction patterns.


179-182: LGTM! FunctionEnvironment context conversion.

Using context_rs.get().into() for the context field is consistent with the changes across other files, replacing the previous as_context_json() approach.


262-276: LGTM! DimensionQuery construction for resolve operation.

The conversion chain DimensionQuery::from(Map::from(context_updated)) correctly transforms the Conditions to a Map and then to a DimensionQuery, enabling the resolve API to work with the new context format.

crates/frontend/src/pages/experiment_list.rs (1)

104-108: LGTM! Removed dimension filtering to enable resolve mode functionality.

The removal of the variantIds filter here is intentional, as the filtering logic has been moved to context_form.rs (lines 191-198) where it conditionally includes/excludes the dimension based on resolve_mode. This allows experiment list views that use resolve_mode=true to access the variantIds dimension.

crates/frontend/src/pages/experiment_group_listing.rs (3)

28-28: LGTM! Consistent import naming.

Importing Condition directly instead of aliasing ConditionComponent aligns with the naming convention used across other components in this PR.


152-156: LGTM! Conditions construction and component usage updated.

The use of Conditions::from_iter(context) leverages the FromIterator<(String, Value)> implementation from logic.rs (lines 63-69 in the relevant snippets) to construct conditions from the context map. The component rename to Condition is consistent with the codebase.


265-273: LGTM! Dimension data access simplified.

The change to directly use .data without filtering follows the same pattern as experiment_list.rs, delegating dimension filtering to the form components based on resolve_mode.

crates/frontend/src/pages/experiment_list/utils.rs (2)

3-3: Direct Condition import looks consistent.


124-128: Map-based Conditions construction looks good.
Conditions::from_iter(context) and the direct <Condition> usage align with the new context flow.

crates/frontend/src/pages/compare_overrides.rs (4)

20-21: Direct Condition import is consistent with the component update.


74-80: Condition rendering with resolve_summary looks good.


191-193: FunctionEnvironment context mapping is aligned with the new Conditions→Map flow.


272-273: Context conversion via Map::from(context) looks correct.

crates/frontend/src/pages/experiment_groups.rs (3)

23-23: Direct Condition import is fine.


91-95: Context→Conditions conversion in table rows is consistent.


127-133: Group context rendering update looks clean.

crates/frontend/src/pages/experiment_list/filter.rs (4)

28-28: Using Conditions in filters aligns with the refactor.


113-129: Filter summary Conditions construction and resolve summary display look good.


286-292: Context buffer initialization and environment mapping look consistent.


317-317: Context→DimensionQuery conversion is consistent with the new flow.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@ayushjain17 ayushjain17 added the UI things relating to the UI label Jan 16, 2026
v.variable.clone(),
Operator::from(&v.expression),
v.expression.to_value().html_display(),
if v.variable == "variantIds" && resolve_summary {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if v.variable == "variantIds" && resolve_summary {
match v.variable.as_str() {
"variantIds" if resolve_summary => "in",
"variantIds" => "has",
_ => "==",
},

Could we use match with guards here for idiomatic Rust?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

UI things relating to the UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants