fix: audit operator fixes for Dart and Flutter SDKs#8
Open
Conversation
… tests - Add optional httpClient parameter to DefaultHTTPClient for dependency injection - Rewrite tests to use Mockito mocks instead of real HTTP calls to jsonplaceholder.typicode.com - Add comprehensive test coverage for retry logic (502/503), error handling (4xx), and connection failures - Tests are now deterministic and don't depend on external services
- Add attrsSeq tracking to detect attribute changes after assignment - Add audienceMatches() method to check if audience match status changed - Clear assignment cache when audience mismatch status changes - Update example pubspec.lock
Add absmartly_dart.dart as the main entry point that exports all public APIs including SDK classes, configurations, providers, handlers, and JSON models.
- Copy all tests from Flutter SDK to dart_sdk package - Update imports from package:absmartly_sdk to package:absmartly_dart/src/ - Replace flutter_test with test package - Remove Flutter-specific bindings (WidgetsFlutterBinding) - Update test_utils.dart to use dart:io instead of Flutter's rootBundle - Copy test resources (JSON fixtures) for tests - Regenerate mockito mocks for new package structure - All 215 tests pass
Move the example Flutter app from the root example/ directory to packages/flutter_sdk/example/. Also simplify imports to use the single barrel file import instead of individual file imports.
Add declarative widgets for A/B testing in Flutter: - ABSmartlyProvider: InheritedWidget for context propagation - Treatment: Map-based variant rendering - TreatmentBuilder: Builder pattern with variables access - TreatmentSwitch/TreatmentVariant: Declarative child-based variants - LoadingBehavior: Configurable loading state handling All widgets support: - Custom loading widgets - Timeout-based fallback to control - Silent error handling (fallback to control on failure) - Optional custom context override Includes 62 passing tests covering all widget functionality.
- Add VariableValue widget export to flutter_sdk - Fix context assignment cache clearing on refresh - Remove duplicate unit handling in context - Enhance audience_matcher tests with comprehensive coverage
- Add Flutter widget integration tests - Add error recovery scenario tests - Add platform-specific edge case tests - Add performance tests - Add integration scenario tests Total: 130 new tests added, all 158 tests pass
Add 22 context tests covering refresh state preservation, exposure queuing after peek, variable value deduplication, close/publish error handling, post-close exceptions, and unknown experiment/variable handling. Fix 9 pre-existing refresh test failures caused by assignmentCache clearing.
Remove accidentally committed .claude/ directory. Fix binary, equals, and in operators for proper type coercion in dart SDK.
WalkthroughThe PR splits the repo into a monorepo with two packages (packages/dart_sdk and packages/flutter_sdk), adds package manifests and analysis configs, creates a Dart public barrel export, moves/renames many imports and jsonexpr operator classes, replaces Flutter equality helpers with package:collection, introduces ContextPublisher and DefaultContextPublisher (with deprecated aliases), extends Context lifecycle and custom-field APIs, adds Flutter widgets/providers (ABSmartlyProvider, Treatment, TreatmentBuilder, TreatmentSwitch, VariableValue) with loading behaviours, adjusts HTTP client signatures, adds many tests and test resources, updates README/docs, and updates .gitignore entries. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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 unit tests (beta)
📝 Coding Plan
Comment |
Add static create() methods with named parameters to ClientConfig, ABSmartlyConfig, ContextConfig and DefaultHTTPClientConfig. Reduces cascade verbosity while maintaining full backward compatibility.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dart_sdk/test/jsonexpr/operator_test.dart (1)
162-179:⚠️ Potential issue | 🔴 CriticalOperand order is MISALIGNED with Java/JavaScript SDKs – requires correction.
The Dart
InOperatorcurrently evaluates operands as[needle, haystack](lhs=needle, rhs=haystack), but Java and JavaScript SDKs both use[haystack, needle]order. This inconsistency breaks cross-SDK compatibility. The test operands[{"value": "am"}, {"var": "value"}]work in Dart because "Hamza".contains("am") returns true, but the same expression would fail in Java/JavaScript, which expect the haystack (value: "Hamza") first. Either swap the operands in the test or correct theInOperator.binary()implementation to match Java/JavaScript operand order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/jsonexpr/operator_test.dart` around lines 162 - 179, The test reveals that operand order for the 'in' operator is reversed in Dart; update the InOperator.binary() implementation so it treats the first operand (lhs) as the haystack and the second (rhs) as the needle (matching Java/JS behavior used by JsonExpr().evaluateExpr()), then run the existing operator_test.dart test (which passes haystack as {"var":"value"} and needle as {"value":"am"}) to confirm cross-SDK compatibility; adjust only InOperator.binary() logic (not the test) to swap the operands semantically when performing the containment check.
🟡 Minor comments (7)
packages/flutter_sdk/test/platform_test.dart-227-238 (1)
227-238:⚠️ Potential issue | 🟡 MinorTest assertion doesn't verify the stated behaviour.
The test name claims to verify that
waitUntilReadyreturns the same future when called multiple times, but the assertions only check that both futures are not null. To properly test this behaviour, useidentical(future1, future2)orexpect(future1, same(future2)).🔧 Proposed fix
final future1 = testContext.waitUntilReady(); final future2 = testContext.waitUntilReady(); - expect(future1, isNotNull); - expect(future2, isNotNull); + expect(future1, same(future2));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/platform_test.dart` around lines 227 - 238, The test "context waitUntilReady returns same future when called multiple times" currently only asserts non-null; change the assertions to verify the two futures are identical by using expect(future1, same(future2)) (or identical(future1, future2)) so the testContext.waitUntilReady() call actually returns the same Future instance for future1 and future2; update the assertions for future1/future2 accordingly and remove the redundant isNotNull checks.packages/flutter_sdk/test/platform_test.dart-12-25 (1)
12-25:⚠️ Potential issue | 🟡 MinorAdd a
tearDownblock to close Context resources.The
setUpcreates aContextinstance, which has aclose()async method that clears timers and flushes pending operations. Without a correspondingtearDownthat callscontext.close(), these resources won't be cleaned up between test runs, potentially causing test isolation issues or resource leaks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/platform_test.dart` around lines 12 - 25, Add a tearDown block that awaits closing the Context created in setUp to release timers and flush pending operations: call await context.close() (referencing the context variable created via sdk.createContext and the Context.close() method) inside a tearDown() so each test run cleans up resources; ensure the tearDown runs after tests and handles the async close correctly (use tearDown(() async { await context.close(); }) or equivalent).packages/flutter_sdk/test/platform_test.dart-475-482 (1)
475-482:⚠️ Potential issue | 🟡 MinorTest name is misleading – the exception comes from context not being ready, not from long experiment names.
The test creates a context without loading data from the server. When
peekTreatmentis called, it throws an exception fromcheckReady()because the context is not ready, not because of the long name. There is no validation in the codebase for experiment name length. Rename to clarify what is actually being tested, for example:throws exception when context is not ready.If you want a more specific exception matcher, use
throwsA(isA<Exception>())to match the exception type actually thrown by the codebase (notArgumentError).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/platform_test.dart` around lines 475 - 482, The test name and expectation are misleading because the exception is thrown by Context.checkReady(), not by experiment name length; rename the test from "handles very long experiment names" to something like "throws exception when context is not ready", and update the assertion to match the actual exception type (use throwsA(isA<Exception>()) instead of throwsException or ArgumentError) while leaving the setup using ContextConfig.create(), sdk.createContext(...), and testContext.peekTreatment(...) intact.docs/plans/2026-01-21-flutter-widgets-design.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorRemove the assistant-specific note from the checked-in plan.
Line 3 is an internal execution instruction rather than product documentation. It reads like another accidentally tracked tooling artefact and will confuse anyone using this plan as the source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/2026-01-21-flutter-widgets-design.md` at line 3, Remove the assistant-specific/internal instruction line that reads "For Claude: REQUIRED SUB-SKILL: Use superpowers:executing-plans" from the checked-in plan document so the plan contains only product-facing content; edit the file to delete that exact line (referenced string "For Claude: REQUIRED SUB-SKILL: Use superpowers:executing-plans") and commit the change with a note indicating removal of an internal execution artifact.packages/dart_sdk/test/context_test.dart-2112-2126 (1)
2112-2126:⚠️ Potential issue | 🟡 MinorTest name contradicts expected behaviour.
The test is named
publishDoesNotClearQueueOnFailure, but line 2125 asserts thatgetPendingCount()equals 0, which means the queue was cleared. Either the test name is incorrect, or the assertion should expect the queue to retain its pending count after failure.🐛 Suggested fix if queue should be retained on failure
- expect(context.getPendingCount(), equals(0)); + expect(context.getPendingCount(), equals(1));Or rename the test if the current behaviour is intentional:
- test('publishDoesNotClearQueueOnFailure', () async { + test('publishClearsQueueEvenOnFailure', () async {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/context_test.dart` around lines 2112 - 2126, The test name publishDoesNotClearQueueOnFailure contradicts its assertion; decide intent and fix accordingly: if the queue should be retained on failure, change the final expectation in the test (test 'publishDoesNotClearQueueOnFailure') to expect getPendingCount() equals(1) after calling context.publish(); if the current behavior (clearing the queue) is intended, rename the test to something like publishClearsQueueOnFailure to match the assertion. Locate the test using createReadyContext(), context.publish(), getPendingCount(), and the when(eventHandler.publish...) setup to apply the change.packages/flutter_sdk/test/integration_test.dart-313-342 (1)
313-342:⚠️ Potential issue | 🟡 MinorUnused data from
ABSmartlyProvider.of()- incomplete test verification.The
datavariable retrieved viaABSmartlyProvider.of(ctx)on lines 315 and 327 is not used. The test manually setsparentContextUserandchildContextUserto hardcoded strings rather than extracting values from the actual context. This means the test doesn't verify that the nested providers actually return different contexts.💡 Consider verifying actual context data
Builder( builder: (ctx) { final data = ABSmartlyProvider.of(ctx); - parentContextUser = 'parent'; - return Text('Parent: $parentContextUser', + // Verify we got the parent context by checking the unit + final unit = data.context.getUnit('user_id'); + return Text('Parent: $unit', textDirection: TextDirection.ltr); }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/integration_test.dart` around lines 313 - 342, The test currently ignores the ABSmartlyProvider.of(ctx) return value (variable data) and instead assigns hardcoded strings to parentContextUser and childContextUser, so update the two Builder closures to extract the relevant value from data (using the provider's API on the returned object) and assign that to parentContextUser and childContextUser respectively, then assert those extracted values (rather than hardcoded strings) to verify the nested providers return different contexts; locate the usages in the two Builder builders where ABSmartlyProvider.of(ctx) is called and replace the manual assignments with reads from data (e.g., data.<contextField> or data.context.<field>) before the Text() and assertions.packages/flutter_sdk/test/error_recovery_test.dart-101-128 (1)
101-128:⚠️ Potential issue | 🟡 MinorTest lacks assertion after timeout elapses.
This test pumps past the
readyTimeoutduration but doesn't verify the expected widget state afterwards. Consider adding an assertion to confirm the control variant is displayed after the timeout.💡 Suggested addition
await tester.pump(); await tester.pump(const Duration(milliseconds: 100)); + + // After timeout, should show control variant + expect(find.text('Control After Timeout'), findsOneWidget); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/error_recovery_test.dart` around lines 101 - 128, The test currently advances past readyTimeout but never asserts the expected UI change; after the final await tester.pump(const Duration(milliseconds: 100)); add an assertion that the control variant is shown (for example using expect(find.text('Control After Timeout'), findsOneWidget) or expect(find.byWidgetPredicate(...) , findsOneWidget)) to verify ABSmartlyProvider + Treatment with defaultLoadingBehavior: LoadingBehavior.placeholder and readyTimeout displays the control after timeout.
🧹 Nitpick comments (18)
packages/dart_sdk/test/resources/custom_fields_context.json (1)
136-136: Prefer omittingcustomFieldValuesfor the no-fields fixture.Line 136 models an explicit
null, not a missing field. Forexp_test_no_custom_fields, that means this fixture does not cover the deserialisation path wherecustomFieldValuesis absent altogether, which is usually the more valuable compatibility case for API payloads.♻️ Suggested fixture tweak
- "audience": null, - "customFieldValues": null + "audience": null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/resources/custom_fields_context.json` at line 136, The fixture exp_test_no_custom_fields currently includes "customFieldValues": null which models an explicit null instead of an absent field; remove the entire "customFieldValues" entry from that JSON object so the fixture omits the key entirely, ensuring the deserialisation path for a missing customFieldValues is covered..gitignore (1)
36-36: Remove the duplicate.DS_Storeentry.
.DS_Storeis already ignored at Line 6, so this second rule is redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 36, Remove the duplicate .DS_Store entry from the .gitignore by deleting the redundant ".DS_Store" line (the file already contains a ".DS_Store" rule earlier), leaving only the single existing ignore rule for .DS_Store to avoid redundancy.packages/dart_sdk/test/ab_smartly_test.dart (1)
3-16: Consider importing from the public barrel file instead of directly fromsrc/.All imported types are exported from
package:absmartly_dart/absmartly_dart.dart, so the imports can be simplified to use the public API rather than directly accessing the internalsrc/directory. For example:import 'package:absmartly_dart/absmartly_dart.dart';This decouples the test from the internal package structure and relies on the public API contract instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/ab_smartly_test.dart` around lines 3 - 16, Replace the many direct internal imports (e.g., ab_smartly.dart, absmartly_sdk_config.dart, client.dart, context.dart, context_config.dart, context_data_provider.dart, context_event_handler.dart, context_event_logger.dart, default_context_data_provider.dart, default_context_event_handler.dart, json/context_data.dart, variable_parser.dart, audience_deserializer.dart) with the package's public barrel import by using import 'package:absmartly_dart/absmartly_dart.dart'; so the test relies on the public API rather than internal src/ files and still has access to AbSmartly, ABSmartlySdkConfig, Client, Context, ContextConfig, ContextDataProvider, ContextEventHandler, ContextEventLogger, DefaultContextDataProvider, DefaultContextEventHandler, ContextData, VariableParser, AudienceDeserializer, etc.packages/flutter_sdk/test/platform_test.dart (2)
107-124: Test lacks assertions for directionality behaviour.The test name suggests it validates that widgets handle directionality correctly, but it only pumps the widget without asserting any RTL-specific behaviour. Consider adding assertions that verify the expected layout or text direction is respected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/platform_test.dart` around lines 107 - 124, The test currently only pumps the widget tree; add explicit assertions to verify RTL is applied by checking the Directionality and a descendant widget: after pumpWidget/pump, assert that the Directionality widget (find.byType(Directionality) or Directionality.of from the Treatment context) has textDirection == TextDirection.rtl and also assert a child widget respects RTL (e.g., verify a Text/TextAlign or layout order expected under RTL within the Treatment) using tester.widget/find to locate the child and expect the RTL-specific property.
181-201: Test name doesn't reflect actual behaviour being tested.The test is named
widgets handle dark/light theme contextbut it only renders a widget with a hardcoded colour. It doesn't test actual theme switching or theme-dependent rendering. Consider either renaming to reflect what's actually being tested (e.g.,widgets render ColoredBox correctly) or enhancing to test actual theme integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/platform_test.dart` around lines 181 - 201, The test name misstates its intent: update the testWidgets named 'widgets handle dark/light theme context' to match what it's doing (e.g., rename to 'widgets render ColoredBox correctly') or modify the test to actually exercise theme behavior by wrapping the ABSmartlyProvider/Treatment in a MaterialApp with ThemeData.light()/ThemeData.dark(), render a widget that reads Theme.of(context) (or uses Theme-dependent colors) and assert different outputs for light vs dark; reference the testWidgets function, ABSmartlyProvider, Treatment(name: 'theme_experiment') and the rendered widget (ColoredBox/Text) so the change either renames the test or adds a second test that toggles ThemeData and asserts theme-specific rendering.packages/dart_sdk/lib/src/json/context_data.dart (1)
11-19: Consider aligninghashCodewith deep equality semantics.The
operator==now usesListEqualityfor content-based comparison, buthashCodestill uses the list's identity hash viaexperiments.hashCode. This can violate the equals/hashCode contract when twoContextDatainstances contain equal experiments in different list instances.♻️ Suggested fix using ListEquality for hashCode
`@override` int get hashCode { - return experiments.hashCode; + return const ListEquality().hash(experiments); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/lib/src/json/context_data.dart` around lines 11 - 19, The hashCode currently uses the list's identity hash which conflicts with operator== that uses const ListEquality() for deep comparison; update ContextData.hashCode to compute a content-based hash using the same ListEquality (e.g. return const ListEquality().hash(experiments)) so hashCode aligns with operator== for the experiments field and preserves the equals/hashCode contract.packages/dart_sdk/lib/src/client_config.dart (1)
105-107: Redundantlatemodifier on nullable fields.The
latekeyword on nullable fields (String?) is redundant and potentially misleading.lateis typically used to defer initialisation of non-nullable fields, but these fields are already nullable and have implicitnulldefaults.🔧 Suggested fix
- late String? apiKey_; - late String? environment_; - late String? application_; + String? apiKey_; + String? environment_; + String? application_;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/lib/src/client_config.dart` around lines 105 - 107, Remove the redundant late modifier from the nullable fields apiKey_, environment_, and application_ in the client configuration class: these are declared as String? so they already default to null and don't need late; update their declarations to plain nullable fields (e.g., String? apiKey_) in the same class where apiKey_, environment_, and application_ are defined.packages/flutter_sdk/lib/absmartly_sdk.dart (1)
8-8: Consider keeping the inherited-widget layer private.If this export is only here to surface
ABSmartlyData, it would be cleaner to expose that type from a dedicated public library and keep the raw inherited-widget implementation internal. Exportingsrc/inherited_absmartly.darthere makes that implementation detail part of the stable API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/lib/absmartly_sdk.dart` at line 8, The package currently exports the internal inherited-widget implementation via "src/inherited_absmartly.dart"; remove that export from the public surface and instead expose only the public type needed (e.g., ABSmartlyData) from a dedicated public API surface (either by adding an export for a public file that re-exports ABSmartlyData or by exporting that type directly from this library), keeping the actual inherited widget class internal to src so the implementation detail is not part of the stable API; update references to ABSmartlyData consumers to import the public symbol rather than the src file.packages/dart_sdk/test/ab_smartly_config_test.dart (1)
58-79: Exercise theaudienceDeserializerbranch as well.The new
ABSmartlyConfig.create()test covers every added named parameter exceptaudienceDeserializer, so that setter path can break without any test signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/ab_smartly_config_test.dart` around lines 58 - 79, The test for ABSmartlyConfig.create is missing coverage for the audienceDeserializer named parameter; update the test in ab_smartly_config_test.dart to pass a mock/fake audience deserializer into ABSmartlyConfig.create (e.g., create MockAudienceDeserializer) and add an expect asserting config.getAudienceDeserializer() equals that mock, ensuring the audienceDeserializer setter/getter path (in ABSmartlyConfig.create and the corresponding getAudienceDeserializer method) is exercised.packages/dart_sdk/test/context_config_test.dart (1)
66-89: CovercontextEventLoggerin the new factory test.
ContextConfig.create()gained this named argument, but the new group never sets or reads it. A wiring regression in that branch would still leave the suite green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/context_config_test.dart` around lines 66 - 89, The test for ContextConfig.create is missing coverage for the new named parameter contextEventLogger; update the 'create with all parameters' test to pass a sentinel logger object into ContextConfig.create via the contextEventLogger argument and assert the resulting config exposes it (e.g., via the accessor on the ContextConfig instance such as getContextEventLogger or the contextEventLogger property) so the factory wiring is exercised and validated.packages/flutter_sdk/test/absmartly_sdk_test.dart (1)
90-147: Exercise the new factory surface from the Flutter package.These cases still go through setter chaining and mostly assert non-null, so the Flutter-side suite would not catch
ABSmartlyConfig.create(...)orContextConfig.create(...)regressing in the wrapper export. Mirroring the named-parameter assertions here would make the cross-SDK coverage much more meaningful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/absmartly_sdk_test.dart` around lines 90 - 147, Tests only exercise the setter-chaining surface and miss asserting the new factory overloads; update the tests to call ABSmartlyConfig.create(...) with its named parameters (e.g., client: Client.create(...), apiKey/app/environment equivalents if exposed) and assert the created object's fields reflect those inputs, and likewise call ContextConfig.create(...) with named parameters (e.g., units: {...}, publishDelay:, refreshInterval:) and assert the resulting ContextConfig contains those values—modify the ABSmartlyConfig and ContextConfig test cases to add these factory-parameter assertions in addition to the existing setter-based tests so regressions in the wrapper export are caught.README.md (2)
299-304: Add language specifier to code block.The fenced code block is missing a language specifier, which affects syntax highlighting and linting compliance.
📝 Suggested fix
-``` +```dart context.track("payment", {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 299 - 304, The fenced code block around the example that calls context.track("payment", {...}) is missing a language specifier; update the opening fence to include the language (e.g., change ``` to ```dart) so the block reads as a Dart snippet and enables proper syntax highlighting and linting for the context.track("payment", { "item_count": 1, "total_amount": 1999.99, }); example.
350-356: Add language specifier to code block.This fenced code block is also missing a language specifier.
📝 Suggested fix
-``` +```dart final contextConfig = ContextConfig.create()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 350 - 356, The fenced code block showing the ContextConfig example lacks a language specifier; update the block to use "```dart" at the opening fence so syntax highlighting applies to the snippet containing ContextConfig.create(), setUnit(...), setContextEventLogger(CustomEventLogger()) and related calls.packages/flutter_sdk/lib/src/widgets/absmartly_provider.dart (1)
132-138: Context close indispose()is not awaited.The
_context.close()call on line 135 returns aFuture<void>that is not awaited. Whiledispose()is synchronous by design, ignoring the future means any errors during close will be silently dropped, and the close operation may not complete before the widget is fully disposed.Consider using
unawaited()fromdart:asyncto make the intent explicit, or add error handling to prevent silent failures.♻️ Suggested improvement
+import 'dart:async'; + `@override` void dispose() { if (_isInternallyCreated) { - _context.close(); + unawaited(_context.close().catchError((_) { + // Log or handle close errors if needed + })); } super.dispose(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/lib/src/widgets/absmartly_provider.dart` around lines 132 - 138, The dispose() method currently calls _context.close() without awaiting its Future; update dispose() to explicitly handle the async close by either calling unawaited(_context.close()) after importing unawaited from dart:async or by calling _context.close().catchError(...) to surface/log errors; target the dispose() implementation and the symbols _isInternallyCreated and _context.close() to ensure the intent to not await is explicit and errors are handled.packages/flutter_sdk/test/integration_test.dart (1)
674-700: Unuseddatavariable from provider lookup.The
datavariable fromABSmartlyProvider.of(ctx)at line 676 is retrieved but never used. If the intent is to verify context attributes are accessible, consider adding assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/integration_test.dart` around lines 674 - 700, The variable "data" retrieved via ABSmartlyProvider.of(ctx) inside the Builder's builder is unused; either remove that unused lookup or use it for assertions/validations. Update the Builder (builder callback) to either delete the line "final data = ABSmartlyProvider.of(ctx);" if not needed, or replace it with meaningful checks against the provider (e.g., assert on expected provider properties) so the provider lookup via ABSmartlyProvider.of(ctx) is actually consumed.packages/flutter_sdk/lib/src/widgets/treatment.dart (1)
81-103: Potential race condition withwidget.namein async callback.In
_initializeTreatment,ctxis captured before the async operation, butwidget.nameis accessed in_updateTreatmentafterwaitUntilReady()completes. IfdidUpdateWidgetfires during this async wait,widget.namemay have changed.Although
didUpdateWidgetdoes call_initializeTreatmentagain (which would start a new operation), the old callback could still run if it completes before the new one, leading to a brief flash of the wrong treatment.Consider capturing
widget.nameat the start:♻️ Suggested fix
void _initializeTreatment() { final ctx = _context; + final experimentName = widget.name; if (ctx.isReady()) { - _updateTreatment(ctx); + _updateTreatment(ctx, experimentName); } else { _startTimeoutIfNeeded(); ctx.waitUntilReady().then((_) { if (mounted) { _cancelTimeout(); - _updateTreatment(ctx); + _updateTreatment(ctx, experimentName); } }).catchError((_) {Then update
_updateTreatment:- void _updateTreatment(Context ctx) { + void _updateTreatment(Context ctx, String experimentName) { setState(() { _isReady = true; - _variant = ctx.isFailed() ? 0 : ctx.getTreatment(widget.name); + _variant = ctx.isFailed() ? 0 : ctx.getTreatment(experimentName); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/lib/src/widgets/treatment.dart` around lines 81 - 103, Capture the current widget.name before starting the async wait in _initializeTreatment and use that captured value when calling _updateTreatment (or pass it as a parameter) so the async callback cannot apply a treatment for a stale name; alternatively, in the waitUntilReady().then callback compare the captured name to the current widget.name and abort if they differ. Update references in _updateTreatment (or add a new parameter) to accept and use the capturedName, and ensure didUpdateWidget() behavior remains unchanged.packages/flutter_sdk/test/performance_test.dart (2)
176-190: Memory test validates context creation but doesn't verify memory usage.The test "creating multiple contexts does not cause memory issues" only verifies that 50 contexts can be created and are non-null. It doesn't actually measure memory consumption. Consider renaming to "supports creating multiple contexts" for accuracy, or add actual memory profiling if memory validation is intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/performance_test.dart` around lines 176 - 190, The test named "creating multiple contexts does not cause memory issues" only asserts creation and non-nullness of 50 Context instances (using Context, ContextConfig.create(), and sdk.createContext) but doesn't measure memory; either rename the test to "supports creating multiple contexts" to accurately reflect its assertions, or replace/extend it to perform real memory checks (e.g., use a memory profiler or capture heap before/after and assert no unexpected growth) while keeping the ContextConfig and sdk.createContext usage intact; update the test description string and/or add the memory-measurement logic accordingly.
431-502: Timing-based assertions may cause flaky tests in CI.The performance benchmarks use hardcoded timing thresholds (e.g.,
lessThan(1000)ms,lessThan(500)ms). These may pass locally but fail intermittently on slower CI runners or under resource contention.Consider either:
- Increasing thresholds with generous margins for CI variability
- Marking these as integration/benchmark tests run separately from unit tests
- Using relative performance comparisons rather than absolute thresholds
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/performance_test.dart` around lines 431 - 502, The timing-based assertions in the "Performance Benchmarks" group (tests: "context creation is fast", "attribute setting is fast", "override setting is fast", and the testWidgets "widget rendering is efficient") use hard thresholds measured by Stopwatch and are flaky on CI; either relax those expectations (increase the lessThan values to generous margins), or convert these into non-unit/benchmark tests by tagging/skipping them in regular runs and moving them to a separate integration/benchmark suite; update the tests that call ContextConfig.create, sdk.createContext, contextConfig.setAttribute, contextConfig.setOverride, and the TestWidgets using ABSmartlyProvider/Treatment to use the new thresholds or to be annotated (skip/Tags) so they do not run in the normal unit test pipeline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dart_sdk/lib/src/context.dart`:
- Around line 725-729: The current setData() implementation clears
assignmentCache_ which drops Assignment.exposed state and breaks exposure
deduplication; update setData() to preserve exposed flags by merging existing
assignmentCache_ entries into the new cache instead of clearing it outright:
when replacing index_, indexVariables_, and data_ in setData(), iterate existing
assignmentCache_ and for each key that still applies to the new data keep its
Assignment.exposed (or copy the whole Assignment) into the new assignmentCache_
so subsequent getTreatment() and getVariableValue() calls retain exposure state;
ensure setRefreshTimer() still runs after merging.
In `@packages/dart_sdk/lib/src/json/experiment.dart`:
- Around line 54-57: The Experiment equality now uses deep list equality for
split, trafficSplit, applications, and variants, but the hashCode still derives
from the list instances; update Experiment.hashCode to compute hashes from the
list contents (e.g., use const ListEquality().hash(...) or const
DeepCollectionEquality().hash(...) for each of split, trafficSplit,
applications, variants) and combine them with the other field hashes so hashCode
is consistent with operator==; modify the hashCode getter in the Experiment
class accordingly to call ListEquality().hash for those list fields and keep the
existing combination logic.
In `@packages/dart_sdk/lib/src/json/publish_event.dart`:
- Around line 31-34: The PublishEvent class uses element-wise list comparison in
its operator == (using ListEquality for units, exposures, goals, attributes) but
hashCode still uses list identity; update the hashCode getter in PublishEvent to
compute deep hashes for those lists (e.g. using ListEquality().hash or
DeepCollectionEquality().hash for each of units, exposures, goals, attributes)
and combine them with the existing scalar fields so that equal instances produce
equal hash codes.
In `@packages/dart_sdk/test/default_context_data_deserializer_test.dart`:
- Line 4: The test imports the serializer file but constructs
DefaultContextDataDeserializer; change the import to the deserializer
implementation (import the file that defines DefaultContextDataDeserializer,
e.g., default_context_data_deserializer.dart) so the test compiles and the
DefaultContextDataDeserializer symbol is resolved.
In `@packages/dart_sdk/test/default_http_client_test.dart`:
- Around line 82-87: The POST/PUT Mockito stubs and verifications in
default_http_client_test.dart are matching an `encoding` named argument that
production calls do not pass; update the mock setups and verify calls around the
`when(mockHttpClient.post(...))` and `when(mockHttpClient.put(...))` (and their
corresponding `verify(...)` statements) to remove the `encoding:
anyNamed('encoding')` entries so the mock signatures match the actual calls to
client.post() and client.put(); check all occurrences used for stubbing and
verification (the mockHttpClient.post and mockHttpClient.put blocks) and delete
the `encoding` named-argument matcher there.
In `@packages/flutter_sdk/lib/src/widgets/treatment_builder.dart`:
- Around line 64-73: didUpdateWidget and the async initialization can be
clobbered by stale completions and the provider context lookup misses ambient
changes; before calling _initializeTreatment() in didUpdateWidget reset the
local state flags (_isReady, _variant, _variables) and increment a generation
token stored on the instance, then have the async completion paths returned by
waitUntilReady() verify the token before applying results so stale completions
are ignored; update the _context/_providerData access so provider changes are
observed (either use listen:true when reading ABSmartlyProvider.of or subscribe
to provider changes and call setState to re-run initialization) and ensure
_initializeTreatment() reads the current generation token at start to bind
results to the correct generation.
In `@packages/flutter_sdk/lib/src/widgets/treatment_switch.dart`:
- Around line 197-200: Avoid using the first experimental child as the control
fallback in _getControlChild; change the fallback to a safe empty widget instead
of widget.children.first.child. In function _getControlChild, keep the primary
lookup via _findVariantChild(0) but replace the fallback to return const
SizedBox.shrink() (or another explicit control placeholder) so
control/loading/failure paths never render an experimental branch by accident.
- Around line 99-108: The widget can be left showing a stale treatment when the
ambient ABSmartlyProvider changes because _context and _providerData use listen:
false and previous waitUntilReady callbacks can update state for an old context;
fix this by detecting provider swaps and fully reinitialising/resetting state:
in TreatmentSwitch add logic in didChangeDependencies (or didUpdateWidget) to
compare the current ABSmartlyProvider context (use ABSmartlyProvider.of(context)
to observe changes) against the value returned by the _context getter, and when
it changes cancel any pending waitUntilReady callbacks, reset _isReady,
_timedOut and _variant to initial values, and call the existing
init/reinitialisation path so a new waitUntilReady is attached to the new
provider; additionally, make waitUntilReady callbacks verify that the
provider/context they were registered for still matches the current provider
(e.g. capture the provider/context identity when registering and return early if
it mismatches) before calling _updateTreatment to avoid overwriting with stale
results.
In `@packages/flutter_sdk/lib/src/widgets/variable_value.dart`:
- Around line 66-75: The widget keeps stale async state because didUpdateWidget
doesn’t reset _isReady/_timedOut nor guard against old waitUntilReady callbacks
or ambient provider swaps; update VariableValue.didUpdateWidget to detect
changes to widget.name, widget.context and the provider instance (compare
ABSmartlyProvider.maybeOf(context, listen: false) / ABSmartlyProvider.of lookup
used in the _context/_providerData getters), reset _isReady and _timedOut before
starting a reload, and introduce a cancellation/version token (e.g.,
_reloadVersion or CancelableOperation) that is incremented/used to ignore or
cancel prior async callbacks so old waitUntilReady results cannot overwrite
newer state.
In `@packages/flutter_sdk/pubspec.yaml`:
- Around line 11-15: pubspec.yaml currently declares a local path dependency
"absmartly_dart: path: ../dart_sdk", which prevents publishing to pub.dev;
replace the path dependency with a hosted source (either a published pub.dev
version or a git dependency) by updating the "absmartly_dart" entry, or if this
package should not be published add "publish_to: none" at the top of
pubspec.yaml to mark it private; locate the dependency by the "absmartly_dart"
key in pubspec.yaml and modify either the dependency source or add the
"publish_to" field accordingly.
- Around line 7-9: The Flutter SDK constraint in the environment block is
incompatible with the Dart SDK constraint: update the flutter entry under the
environment section (currently "flutter: \">=1.17.0\"") to "flutter:
\">=3.3.10\"" so the Flutter version bundles a Dart SDK that satisfies sdk:
'>=2.18.6 <4.0.0'; edit the environment block in pubspec.yaml replacing the
flutter constraint accordingly.
In `@packages/flutter_sdk/README.md`:
- Around line 53-64: The README examples call
sdk.createContext(...).waitUntilReady() which returns Future<Context> but is
used without awaiting; update the "Synchronously" code snippets to await the
future (e.g., use await sdk.createContext(contextConfig).waitUntilReady()) and
ensure the surrounding example shows an async context (e.g., an async function
or main) so the assignment to Context? receives a resolved Context; reference
the symbols ContextConfig.create, sdk.createContext, and waitUntilReady when
making the change.
In `@packages/flutter_sdk/test/widgets/widget_integration_test.dart`:
- Around line 98-129: The test 'listen parameter controls rebuild behavior'
never verifies rebuilds after provider state changes; update it to mutate the
provider (e.g., trigger a state/value change via the ABSmartlyProvider or the
underlying SDK/context) after the initial tester.pumpWidget so that
ABSmartlyProvider.of(ctx, listen: true) should rebuild while
ABSmartlyProvider.of(ctx, listen: false) should not; specifically, after the
initial pump call, perform a provider update (using the same sdk/context or a
provided change method) and call tester.pump() again, then assert that
listenBuildCount increased while noListenBuildCount did not, referencing the
existing listenBuildCount, noListenBuildCount, ABSmartlyProvider.of,
tester.pumpWidget and tester.pump to locate and modify the test.
- Around line 202-221: The test "shows loading widget when provided and context
not ready" currently never asserts the loading UI; update the test that builds
ABSmartlyProvider/Treatment (look for the testWidgets block with name 'shows
loading widget when provided and context not ready') to verify that after the
initial tester.pump() the provided loading widget (the Text 'Loading...') is
present in the widget tree (e.g., expect(find.text('Loading...'),
findsOneWidget), then advance time past the provider timeout (pump with Duration
> 100ms) and assert the control variant is shown (expect(find.text('Control'),
findsOneWidget)); apply the same pattern to the related timeout test (the test
covering lines 242-261) so both loading and timeout fallback behaviors are
actually asserted.
- Around line 159-198: The test "Treatment widget updates when experiment name
changes" never exercises the change path; after verifying the initial label,
simulate tapping the GestureDetector with key 'change_button' (use await
tester.tap(find.byKey(const Key('change_button')))), then call await
tester.pump() (or pumpAndSettle if needed) and assert that Treatment
re-evaluated by expecting find.text('Control for experiment_2') to be found;
keep the existing initial expect for 'Control for experiment_1' then perform the
tap + pump + new expect to validate the Treatment widget updated its experiment
name.
---
Outside diff comments:
In `@packages/dart_sdk/test/jsonexpr/operator_test.dart`:
- Around line 162-179: The test reveals that operand order for the 'in' operator
is reversed in Dart; update the InOperator.binary() implementation so it treats
the first operand (lhs) as the haystack and the second (rhs) as the needle
(matching Java/JS behavior used by JsonExpr().evaluateExpr()), then run the
existing operator_test.dart test (which passes haystack as {"var":"value"} and
needle as {"value":"am"}) to confirm cross-SDK compatibility; adjust only
InOperator.binary() logic (not the test) to swap the operands semantically when
performing the containment check.
---
Minor comments:
In `@docs/plans/2026-01-21-flutter-widgets-design.md`:
- Line 3: Remove the assistant-specific/internal instruction line that reads
"For Claude: REQUIRED SUB-SKILL: Use superpowers:executing-plans" from the
checked-in plan document so the plan contains only product-facing content; edit
the file to delete that exact line (referenced string "For Claude: REQUIRED
SUB-SKILL: Use superpowers:executing-plans") and commit the change with a note
indicating removal of an internal execution artifact.
In `@packages/dart_sdk/test/context_test.dart`:
- Around line 2112-2126: The test name publishDoesNotClearQueueOnFailure
contradicts its assertion; decide intent and fix accordingly: if the queue
should be retained on failure, change the final expectation in the test (test
'publishDoesNotClearQueueOnFailure') to expect getPendingCount() equals(1) after
calling context.publish(); if the current behavior (clearing the queue) is
intended, rename the test to something like publishClearsQueueOnFailure to match
the assertion. Locate the test using createReadyContext(), context.publish(),
getPendingCount(), and the when(eventHandler.publish...) setup to apply the
change.
In `@packages/flutter_sdk/test/error_recovery_test.dart`:
- Around line 101-128: The test currently advances past readyTimeout but never
asserts the expected UI change; after the final await tester.pump(const
Duration(milliseconds: 100)); add an assertion that the control variant is shown
(for example using expect(find.text('Control After Timeout'), findsOneWidget) or
expect(find.byWidgetPredicate(...) , findsOneWidget)) to verify
ABSmartlyProvider + Treatment with defaultLoadingBehavior:
LoadingBehavior.placeholder and readyTimeout displays the control after timeout.
In `@packages/flutter_sdk/test/integration_test.dart`:
- Around line 313-342: The test currently ignores the ABSmartlyProvider.of(ctx)
return value (variable data) and instead assigns hardcoded strings to
parentContextUser and childContextUser, so update the two Builder closures to
extract the relevant value from data (using the provider's API on the returned
object) and assign that to parentContextUser and childContextUser respectively,
then assert those extracted values (rather than hardcoded strings) to verify the
nested providers return different contexts; locate the usages in the two Builder
builders where ABSmartlyProvider.of(ctx) is called and replace the manual
assignments with reads from data (e.g., data.<contextField> or
data.context.<field>) before the Text() and assertions.
In `@packages/flutter_sdk/test/platform_test.dart`:
- Around line 227-238: The test "context waitUntilReady returns same future when
called multiple times" currently only asserts non-null; change the assertions to
verify the two futures are identical by using expect(future1, same(future2)) (or
identical(future1, future2)) so the testContext.waitUntilReady() call actually
returns the same Future instance for future1 and future2; update the assertions
for future1/future2 accordingly and remove the redundant isNotNull checks.
- Around line 12-25: Add a tearDown block that awaits closing the Context
created in setUp to release timers and flush pending operations: call await
context.close() (referencing the context variable created via sdk.createContext
and the Context.close() method) inside a tearDown() so each test run cleans up
resources; ensure the tearDown runs after tests and handles the async close
correctly (use tearDown(() async { await context.close(); }) or equivalent).
- Around line 475-482: The test name and expectation are misleading because the
exception is thrown by Context.checkReady(), not by experiment name length;
rename the test from "handles very long experiment names" to something like
"throws exception when context is not ready", and update the assertion to match
the actual exception type (use throwsA(isA<Exception>()) instead of
throwsException or ArgumentError) while leaving the setup using
ContextConfig.create(), sdk.createContext(...), and
testContext.peekTreatment(...) intact.
---
Nitpick comments:
In @.gitignore:
- Line 36: Remove the duplicate .DS_Store entry from the .gitignore by deleting
the redundant ".DS_Store" line (the file already contains a ".DS_Store" rule
earlier), leaving only the single existing ignore rule for .DS_Store to avoid
redundancy.
In `@packages/dart_sdk/lib/src/client_config.dart`:
- Around line 105-107: Remove the redundant late modifier from the nullable
fields apiKey_, environment_, and application_ in the client configuration
class: these are declared as String? so they already default to null and don't
need late; update their declarations to plain nullable fields (e.g., String?
apiKey_) in the same class where apiKey_, environment_, and application_ are
defined.
In `@packages/dart_sdk/lib/src/json/context_data.dart`:
- Around line 11-19: The hashCode currently uses the list's identity hash which
conflicts with operator== that uses const ListEquality() for deep comparison;
update ContextData.hashCode to compute a content-based hash using the same
ListEquality (e.g. return const ListEquality().hash(experiments)) so hashCode
aligns with operator== for the experiments field and preserves the
equals/hashCode contract.
In `@packages/dart_sdk/test/ab_smartly_config_test.dart`:
- Around line 58-79: The test for ABSmartlyConfig.create is missing coverage for
the audienceDeserializer named parameter; update the test in
ab_smartly_config_test.dart to pass a mock/fake audience deserializer into
ABSmartlyConfig.create (e.g., create MockAudienceDeserializer) and add an expect
asserting config.getAudienceDeserializer() equals that mock, ensuring the
audienceDeserializer setter/getter path (in ABSmartlyConfig.create and the
corresponding getAudienceDeserializer method) is exercised.
In `@packages/dart_sdk/test/ab_smartly_test.dart`:
- Around line 3-16: Replace the many direct internal imports (e.g.,
ab_smartly.dart, absmartly_sdk_config.dart, client.dart, context.dart,
context_config.dart, context_data_provider.dart, context_event_handler.dart,
context_event_logger.dart, default_context_data_provider.dart,
default_context_event_handler.dart, json/context_data.dart,
variable_parser.dart, audience_deserializer.dart) with the package's public
barrel import by using import 'package:absmartly_dart/absmartly_dart.dart'; so
the test relies on the public API rather than internal src/ files and still has
access to AbSmartly, ABSmartlySdkConfig, Client, Context, ContextConfig,
ContextDataProvider, ContextEventHandler, ContextEventLogger,
DefaultContextDataProvider, DefaultContextEventHandler, ContextData,
VariableParser, AudienceDeserializer, etc.
In `@packages/dart_sdk/test/context_config_test.dart`:
- Around line 66-89: The test for ContextConfig.create is missing coverage for
the new named parameter contextEventLogger; update the 'create with all
parameters' test to pass a sentinel logger object into ContextConfig.create via
the contextEventLogger argument and assert the resulting config exposes it
(e.g., via the accessor on the ContextConfig instance such as
getContextEventLogger or the contextEventLogger property) so the factory wiring
is exercised and validated.
In `@packages/dart_sdk/test/resources/custom_fields_context.json`:
- Line 136: The fixture exp_test_no_custom_fields currently includes
"customFieldValues": null which models an explicit null instead of an absent
field; remove the entire "customFieldValues" entry from that JSON object so the
fixture omits the key entirely, ensuring the deserialisation path for a missing
customFieldValues is covered.
In `@packages/flutter_sdk/lib/absmartly_sdk.dart`:
- Line 8: The package currently exports the internal inherited-widget
implementation via "src/inherited_absmartly.dart"; remove that export from the
public surface and instead expose only the public type needed (e.g.,
ABSmartlyData) from a dedicated public API surface (either by adding an export
for a public file that re-exports ABSmartlyData or by exporting that type
directly from this library), keeping the actual inherited widget class internal
to src so the implementation detail is not part of the stable API; update
references to ABSmartlyData consumers to import the public symbol rather than
the src file.
In `@packages/flutter_sdk/lib/src/widgets/absmartly_provider.dart`:
- Around line 132-138: The dispose() method currently calls _context.close()
without awaiting its Future; update dispose() to explicitly handle the async
close by either calling unawaited(_context.close()) after importing unawaited
from dart:async or by calling _context.close().catchError(...) to surface/log
errors; target the dispose() implementation and the symbols _isInternallyCreated
and _context.close() to ensure the intent to not await is explicit and errors
are handled.
In `@packages/flutter_sdk/lib/src/widgets/treatment.dart`:
- Around line 81-103: Capture the current widget.name before starting the async
wait in _initializeTreatment and use that captured value when calling
_updateTreatment (or pass it as a parameter) so the async callback cannot apply
a treatment for a stale name; alternatively, in the waitUntilReady().then
callback compare the captured name to the current widget.name and abort if they
differ. Update references in _updateTreatment (or add a new parameter) to accept
and use the capturedName, and ensure didUpdateWidget() behavior remains
unchanged.
In `@packages/flutter_sdk/test/absmartly_sdk_test.dart`:
- Around line 90-147: Tests only exercise the setter-chaining surface and miss
asserting the new factory overloads; update the tests to call
ABSmartlyConfig.create(...) with its named parameters (e.g., client:
Client.create(...), apiKey/app/environment equivalents if exposed) and assert
the created object's fields reflect those inputs, and likewise call
ContextConfig.create(...) with named parameters (e.g., units: {...},
publishDelay:, refreshInterval:) and assert the resulting ContextConfig contains
those values—modify the ABSmartlyConfig and ContextConfig test cases to add
these factory-parameter assertions in addition to the existing setter-based
tests so regressions in the wrapper export are caught.
In `@packages/flutter_sdk/test/integration_test.dart`:
- Around line 674-700: The variable "data" retrieved via
ABSmartlyProvider.of(ctx) inside the Builder's builder is unused; either remove
that unused lookup or use it for assertions/validations. Update the Builder
(builder callback) to either delete the line "final data =
ABSmartlyProvider.of(ctx);" if not needed, or replace it with meaningful checks
against the provider (e.g., assert on expected provider properties) so the
provider lookup via ABSmartlyProvider.of(ctx) is actually consumed.
In `@packages/flutter_sdk/test/performance_test.dart`:
- Around line 176-190: The test named "creating multiple contexts does not cause
memory issues" only asserts creation and non-nullness of 50 Context instances
(using Context, ContextConfig.create(), and sdk.createContext) but doesn't
measure memory; either rename the test to "supports creating multiple contexts"
to accurately reflect its assertions, or replace/extend it to perform real
memory checks (e.g., use a memory profiler or capture heap before/after and
assert no unexpected growth) while keeping the ContextConfig and
sdk.createContext usage intact; update the test description string and/or add
the memory-measurement logic accordingly.
- Around line 431-502: The timing-based assertions in the "Performance
Benchmarks" group (tests: "context creation is fast", "attribute setting is
fast", "override setting is fast", and the testWidgets "widget rendering is
efficient") use hard thresholds measured by Stopwatch and are flaky on CI;
either relax those expectations (increase the lessThan values to generous
margins), or convert these into non-unit/benchmark tests by tagging/skipping
them in regular runs and moving them to a separate integration/benchmark suite;
update the tests that call ContextConfig.create, sdk.createContext,
contextConfig.setAttribute, contextConfig.setOverride, and the TestWidgets using
ABSmartlyProvider/Treatment to use the new thresholds or to be annotated
(skip/Tags) so they do not run in the normal unit test pipeline.
In `@packages/flutter_sdk/test/platform_test.dart`:
- Around line 107-124: The test currently only pumps the widget tree; add
explicit assertions to verify RTL is applied by checking the Directionality and
a descendant widget: after pumpWidget/pump, assert that the Directionality
widget (find.byType(Directionality) or Directionality.of from the Treatment
context) has textDirection == TextDirection.rtl and also assert a child widget
respects RTL (e.g., verify a Text/TextAlign or layout order expected under RTL
within the Treatment) using tester.widget/find to locate the child and expect
the RTL-specific property.
- Around line 181-201: The test name misstates its intent: update the
testWidgets named 'widgets handle dark/light theme context' to match what it's
doing (e.g., rename to 'widgets render ColoredBox correctly') or modify the test
to actually exercise theme behavior by wrapping the ABSmartlyProvider/Treatment
in a MaterialApp with ThemeData.light()/ThemeData.dark(), render a widget that
reads Theme.of(context) (or uses Theme-dependent colors) and assert different
outputs for light vs dark; reference the testWidgets function,
ABSmartlyProvider, Treatment(name: 'theme_experiment') and the rendered widget
(ColoredBox/Text) so the change either renames the test or adds a second test
that toggles ThemeData and asserts theme-specific rendering.
In `@README.md`:
- Around line 299-304: The fenced code block around the example that calls
context.track("payment", {...}) is missing a language specifier; update the
opening fence to include the language (e.g., change ``` to ```dart) so the block
reads as a Dart snippet and enables proper syntax highlighting and linting for
the context.track("payment", { "item_count": 1, "total_amount": 1999.99, });
example.
- Around line 350-356: The fenced code block showing the ContextConfig example
lacks a language specifier; update the block to use "```dart" at the opening
fence so syntax highlighting applies to the snippet containing
ContextConfig.create(), setUnit(...), setContextEventLogger(CustomEventLogger())
and related calls.
| import 'package:flutter/material.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:absmartly_dart/src/context_data_deserializer.dart'; | ||
| import 'package:absmartly_dart/src/default_context_data_serializer.dart'; |
There was a problem hiding this comment.
Import the deserialiser implementation, not the serialiser.
Line 19 constructs DefaultContextDataDeserializer, but this import points at default_context_data_serializer.dart. That looks like a straight swap typo and will stop the test compiling.
Suggested fix
-import 'package:absmartly_dart/src/default_context_data_serializer.dart';
+import 'package:absmartly_dart/src/default_context_data_deserializer.dart';📝 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
| import 'package:absmartly_dart/src/default_context_data_serializer.dart'; | |
| import 'package:absmartly_dart/src/default_context_data_deserializer.dart'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dart_sdk/test/default_context_data_deserializer_test.dart` at line
4, The test imports the serializer file but constructs
DefaultContextDataDeserializer; change the import to the deserializer
implementation (import the file that defines DefaultContextDataDeserializer,
e.g., default_context_data_deserializer.dart) so the test compiles and the
DefaultContextDataDeserializer symbol is resolved.
Comment on lines
+11
to
+15
| dependencies: | ||
| flutter: | ||
| sdk: flutter | ||
| absmartly_dart: | ||
| path: ../dart_sdk |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can a Dart or Flutter package published to pub.dev include a path:dependency inpubspec.yaml?
💡 Result:
No. A package intended to be published on pub.dev can’t have path: dependencies in its pubspec.yaml. If you try, you’ll get an invalid_dependency-type error; the fix is to change it to a hosted (pub.dev) dependency (or another allowed source like git:), or mark the package as not meant for publishing by adding publish_to: none. [1]
Source:
[1] Dart SDK docs – invalid_dependency diagnostic (explicitly says published packages must not use path: dependencies) (dart.dev)
Citations:
🏁 Script executed:
cat -n packages/flutter_sdk/pubspec.yamlRepository: absmartly/flutter-sdk
Length of output: 724
path: dependency prevents publishing this package to pub.dev.
The package is versioned (2.1.2) and has public repository metadata, indicating publishing intent. However, absmartly_dart is configured with a local path dependency (path: ../dart_sdk), which pub.dev explicitly rejects with an invalid_dependency error. Resolve by changing to a hosted dependency on pub.dev or a git source, or by adding publish_to: none to the manifest if local-only distribution is intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/pubspec.yaml` around lines 11 - 15, pubspec.yaml
currently declares a local path dependency "absmartly_dart: path: ../dart_sdk",
which prevents publishing to pub.dev; replace the path dependency with a hosted
source (either a published pub.dev version or a git dependency) by updating the
"absmartly_dart" entry, or if this package should not be published add
"publish_to: none" at the top of pubspec.yaml to mark it private; locate the
dependency by the "absmartly_dart" key in pubspec.yaml and modify either the
dependency source or add the "publish_to" field accordingly.
Comment on lines
+98
to
+129
| testWidgets('listen parameter controls rebuild behavior', (tester) async { | ||
| int listenBuildCount = 0; | ||
| int noListenBuildCount = 0; | ||
|
|
||
| await tester.pumpWidget( | ||
| ABSmartlyProvider( | ||
| sdk: sdk, | ||
| context: context, | ||
| child: Row( | ||
| textDirection: TextDirection.ltr, | ||
| children: [ | ||
| Builder( | ||
| builder: (ctx) { | ||
| ABSmartlyProvider.of(ctx, listen: true); | ||
| listenBuildCount++; | ||
| return const SizedBox(); | ||
| }, | ||
| ), | ||
| Builder( | ||
| builder: (ctx) { | ||
| ABSmartlyProvider.of(ctx, listen: false); | ||
| noListenBuildCount++; | ||
| return const SizedBox(); | ||
| }, | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| expect(listenBuildCount, greaterThan(0)); | ||
| expect(noListenBuildCount, greaterThan(0)); |
There was a problem hiding this comment.
This test never exercises the listen behaviour.
Both counters are only asserted after the initial pump. Because nothing changes in the provider/context afterwards, a regression where listen: true and listen: false rebuild identically would still pass. Drive a provider update and assert that only the listening branch rebuilds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/test/widgets/widget_integration_test.dart` around lines
98 - 129, The test 'listen parameter controls rebuild behavior' never verifies
rebuilds after provider state changes; update it to mutate the provider (e.g.,
trigger a state/value change via the ABSmartlyProvider or the underlying
SDK/context) after the initial tester.pumpWidget so that
ABSmartlyProvider.of(ctx, listen: true) should rebuild while
ABSmartlyProvider.of(ctx, listen: false) should not; specifically, after the
initial pump call, perform a provider update (using the same sdk/context or a
provided change method) and call tester.pump() again, then assert that
listenBuildCount increased while noListenBuildCount did not, referencing the
existing listenBuildCount, noListenBuildCount, ABSmartlyProvider.of,
tester.pumpWidget and tester.pump to locate and modify the test.
Comment on lines
+159
to
+198
| testWidgets('Treatment widget updates when experiment name changes', | ||
| (tester) async { | ||
| String experimentName = 'experiment_1'; | ||
|
|
||
| await tester.pumpWidget( | ||
| StatefulBuilder( | ||
| builder: (ctx, setState) { | ||
| return ABSmartlyProvider( | ||
| sdk: sdk, | ||
| context: context, | ||
| defaultLoadingBehavior: LoadingBehavior.control, | ||
| child: Column( | ||
| children: [ | ||
| Treatment( | ||
| name: experimentName, | ||
| variants: { | ||
| 0: Text('Control for $experimentName', | ||
| textDirection: TextDirection.ltr), | ||
| }, | ||
| ), | ||
| GestureDetector( | ||
| key: const Key('change_button'), | ||
| onTap: () { | ||
| setState(() { | ||
| experimentName = 'experiment_2'; | ||
| }); | ||
| }, | ||
| child: | ||
| const Text('Change', textDirection: TextDirection.ltr), | ||
| ), | ||
| ], | ||
| ), | ||
| ); | ||
| }, | ||
| ), | ||
| ); | ||
|
|
||
| await tester.pump(); | ||
| expect(find.text('Control for experiment_1'), findsOneWidget); | ||
| }); |
There was a problem hiding this comment.
The experiment-name change path is still untested.
Line 179 wires a change button, but the test never taps it. Also, the rendered label is derived from the local experimentName, so simply asserting the new text would only prove StatefulBuilder rebuilt, not that Treatment re-evaluated the new experiment name.
🧪 Minimum change to exercise the branch
await tester.pump();
expect(find.text('Control for experiment_1'), findsOneWidget);
+
+ await tester.tap(find.byKey(const Key('change_button')));
+ await tester.pump();
+ expect(find.text('Control for experiment_1'), findsNothing);
+ expect(find.text('Control for experiment_2'), findsOneWidget);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/test/widgets/widget_integration_test.dart` around lines
159 - 198, The test "Treatment widget updates when experiment name changes"
never exercises the change path; after verifying the initial label, simulate
tapping the GestureDetector with key 'change_button' (use await
tester.tap(find.byKey(const Key('change_button')))), then call await
tester.pump() (or pumpAndSettle if needed) and assert that Treatment
re-evaluated by expecting find.text('Control for experiment_2') to be found;
keep the existing initial expect for 'Control for experiment_1' then perform the
tap + pump + new expect to validate the Treatment widget updated its experiment
name.
Comment on lines
+202
to
+221
| testWidgets('shows loading widget when provided and context not ready', | ||
| (tester) async { | ||
| await tester.pumpWidget( | ||
| ABSmartlyProvider( | ||
| sdk: sdk, | ||
| context: context, | ||
| defaultLoadingBehavior: LoadingBehavior.placeholder, | ||
| child: Treatment( | ||
| name: 'test_experiment', | ||
| loading: const Text('Loading...', textDirection: TextDirection.ltr), | ||
| variants: { | ||
| 0: const Text('Control', textDirection: TextDirection.ltr), | ||
| 1: const Text('Variant', textDirection: TextDirection.ltr), | ||
| }, | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| await tester.pump(Duration.zero); | ||
| }); |
There was a problem hiding this comment.
These loading-state tests pass without checking the state.
Line 202 never asserts that the loading widget is rendered. Line 242 never pumps past the 100 ms timeout or checks the control fallback. As written, both tests stay green even if the loading/timeout behaviour regresses.
🧪 Suggested assertions
- await tester.pump(Duration.zero);
+ await tester.pump(Duration.zero);
+ expect(find.text('Loading...'), findsOneWidget);
...
- await tester.pump();
+ await tester.pump(const Duration(milliseconds: 101));
+ expect(find.text('Control Fallback'), findsOneWidget);Also applies to: 242-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/test/widgets/widget_integration_test.dart` around lines
202 - 221, The test "shows loading widget when provided and context not ready"
currently never asserts the loading UI; update the test that builds
ABSmartlyProvider/Treatment (look for the testWidgets block with name 'shows
loading widget when provided and context not ready') to verify that after the
initial tester.pump() the provided loading widget (the Text 'Loading...') is
present in the widget tree (e.g., expect(find.text('Loading...'),
findsOneWidget), then advance time past the provider timeout (pump with Duration
> 100ms) and assert the control variant is shown (expect(find.text('Control'),
findsOneWidget)); apply the same pattern to the related timeout test (the test
covering lines 242-261) so both loading and timeout fallback behaviors are
actually asserted.
…, remove override closed check - Fix 1.2: On refresh, selectively remove only changed/stopped experiment assignments from cache instead of clearing all. Preserves exposed=true flag for unchanged experiments so no duplicate exposure events are sent. - Fix 1.3: Clear events (exposures, goals) only after successful publish. On failure, keep events in queue so they can be retried. - Fix 4.1: Remove checkNotClosed() from setOverride/setOverrides to allow setting overrides after context is closed, matching JS SDK behavior.
- Add CustomFieldValue model to Experiment - Implement getCustomFieldKeys(), getCustomFieldValue(), getCustomFieldValueType() - Implement readyError() returning the error from failed data loading - Add unit tests for all new methods
…ndardize error messages Adds finalize(), isFinalized(), and isFinalizing() as aliases for close(), isClosed(), and isClosing() to align with JS SDK terminology. Updates error messages to include trailing periods and use finalized/finalizing language.
…ases and tests Adds camelCase aliases matching the JS SDK API: - customFieldKeys() → getCustomFieldKeys() - customFieldValue(experimentName, key) → getCustomFieldValue(experimentName, key) - customFieldValueType(experimentName, key) → getCustomFieldValueType(experimentName, key) Adds unit tests verifying the aliases return identical results to the get-prefixed methods.
…eady or finalized
Use lowercase 'ABsmartly' prefix and standard unit error formats.
100-widget render time limit relaxed from 5s to 30s to handle slower CI runners
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/dart_sdk/test/context_test.dart (2)
1750-1752:⚠️ Potential issue | 🟡 MinorInconsistent async exception testing pattern.
Same issue as noted above for
publish(). Therefresh()method returns a Future, so exceptions should be caught using the async pattern.🔧 Suggested fix
- expect(() => context.refresh(), throwsA(isA<Exception>())); + await expectLater(context.refresh(), throwsA(isA<Exception>()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/context_test.dart` around lines 1750 - 1752, The test currently uses the synchronous exception pattern expect(() => context.refresh(), ...) but refresh() returns a Future; change it to the async Future pattern by asserting the Future throws, e.g. use expect(context.refresh(), throwsA(isA<Exception>())) or await expectLater(context.refresh(), throwsA(isA<Exception>())), keeping the subsequent verify(dataProvider.getContextData()).called(1); reference the context.refresh() call and the verify(...) assertion when making the change.
1726-1728:⚠️ Potential issue | 🟡 MinorInconsistent async exception testing pattern.
The pattern
expect(() => context.publish(), throwsA(...))won't reliably catch exceptions thrown asynchronously within the Future. Compare with line 2114 which correctly usesawait expectLater(context.publish(), throwsA(...)).🔧 Suggested fix
- expect(() => context.publish(), throwsA(isA<Exception>())); + await expectLater(context.publish(), throwsA(isA<Exception>()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/context_test.dart` around lines 1726 - 1728, The test is using a synchronous exception assertion for an async function; replace expect(() => context.publish(), throwsA(isA<Exception>())) with an asynchronous assertion like await expectLater(context.publish(), throwsA(isA<Exception>())) so the Future returned by context.publish() is awaited and exceptions are caught correctly; keep the subsequent verify(eventHandler.publish(any, any)).called(1) assertion as is.
♻️ Duplicate comments (3)
packages/dart_sdk/lib/src/json/experiment.dart (1)
72-75:⚠️ Potential issue | 🟠 Major
operator==andhashCodeare inconsistent for list fields (still unresolved).Deep list comparison is used in equality, but hash code still uses list instance hashes. This breaks
Set/Mapbehaviour.Suggested fix
class Experiment { + static const ListEquality<double> _doubleListEquality = ListEquality<double>(); + static const ListEquality<ExperimentApplication> _applicationListEquality = + ListEquality<ExperimentApplication>(); + static const ListEquality<ExperimentVariant> _variantListEquality = + ListEquality<ExperimentVariant>(); + `@override` bool operator ==(Object other) { @@ - const ListEquality().equals(split, other.split) && - const ListEquality().equals(trafficSplit, other.trafficSplit) && - const ListEquality().equals(applications, other.applications) && - const ListEquality().equals(variants, other.variants) && + _doubleListEquality.equals(split, other.split) && + _doubleListEquality.equals(trafficSplit, other.trafficSplit) && + _applicationListEquality.equals(applications, other.applications) && + _variantListEquality.equals(variants, other.variants) && audienceStrict == other.audienceStrict && audience == other.audience; } @@ - result = 31 * result + split.hashCode; - result = 31 * result + trafficSplit.hashCode; - result = 31 * result + applications.hashCode; - result = 31 * result + variants.hashCode; + result = 31 * result + _doubleListEquality.hash(split); + result = 31 * result + _doubleListEquality.hash(trafficSplit); + result = 31 * result + _applicationListEquality.hash(applications); + result = 31 * result + _variantListEquality.hash(variants); return result; }#!/bin/bash # Verify remaining equality/hash mismatch in Experiment. rg -n -C2 'ListEquality\(\)\.equals|split\.hashCode|trafficSplit\.hashCode|applications\.hashCode|variants\.hashCode' packages/dart_sdk/lib/src/json/experiment.dartAlso applies to: 84-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/lib/src/json/experiment.dart` around lines 72 - 75, operator== compares list contents using ListEquality/DeepCollectionEquality but hashCode still uses the lists' instance hashCodes; update Experiment.hashCode to compute hashes from list contents (use the same equality type you used in operator==, e.g. const ListEquality().hash(...) or const DeepCollectionEquality().hash(...) for nested lists) for the fields split, trafficSplit, applications, and variants so equality/hashCode are consistent; ensure the collection equality class you pick matches the one used in operator== and call its .hash(...) for each list when composing the overall hash.packages/flutter_sdk/lib/src/widgets/treatment_builder.dart (1)
64-117:⚠️ Potential issue | 🟠 MajorUnresolved: reload path still allows stale async overwrite and misses provider context changes
Same issue remains: Line 84–87 reinitialises without resetting local state, Line 97–115 can apply outdated async completions, and Line 68/72 (
listen: false) does not observe ambient provider context swaps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/lib/src/widgets/treatment_builder.dart` around lines 64 - 117, The widget fails to reset local state when reinitialising and allows stale async completions to overwrite newer state, and it doesn't observe provider swaps because _context and _providerData use listen: false; fix by making _context/_providerData listen to provider changes (use listen: true or rely on context.watch semantics) so ambient provider swaps trigger didUpdateWidget, ensure didUpdateWidget and initState clear/reset local flags (_isReady, _variant, _variables) before calling _initializeTreatment, and protect async completions in _initializeTreatment/_updateTreatment by capturing a unique token or the ctx instance (final token/ctx = _context) and verifying it still matches current _context (or token) and mounted before applying setState; also keep using _startTimeoutIfNeeded/_cancelTimeout but cancel and reset timeouts when reinitialising.packages/flutter_sdk/lib/src/widgets/variable_value.dart (1)
66-113:⚠️ Potential issue | 🟠 MajorReload path is still vulnerable to stale async completions and provider context swaps
This remains unresolved: Line 80–85 reinitialises without resetting
_isReady/_timedOut, and Line 95–112 can apply an olderwaitUntilReady()result after widget updates. Also, Line 70 and Line 74 uselisten: false, so ambient provider context replacement is not observed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/lib/src/widgets/variable_value.dart` around lines 66 - 113, Reset readiness/timed-out state and guard async completions with a generation token when reinitializing: in didUpdateWidget(...) (and any reinit path in _initializeValue) clear _isReady and _timedOut, call _cancelTimeout(), increment a private int token (e.g., _initGeneration) and capture its value in _initializeValue before any async wait; after ctx.waitUntilReady().then(...) or catchError(...) only apply state changes if mounted and the captured token still equals _initGeneration to avoid stale completions; also change the _context getter and _providerData lookup to use the provider with listen: true (e.g., ABSmartlyProvider.of(context, listen: true) / maybeOf(context, listen: true)) so ambient provider swaps trigger widget updates.
🧹 Nitpick comments (5)
packages/flutter_sdk/test/platform_test.dart (3)
295-332: Use addTearDown to ensure StreamController is closed.If the test fails or throws an exception before reaching
controller.close(), the StreamController will remain open. UsingaddTearDownguarantees cleanup regardless of test outcome.♻️ Proposed fix for safer cleanup
testWidgets('StreamBuilder with treatment handles stream events', (tester) async { final controller = StreamController<String>.broadcast(); + addTearDown(() => controller.close()); await tester.pumpWidget( ABSmartlyProvider( ... ), ); ... expect(find.text('Stream: final'), findsOneWidget); - - await controller.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/platform_test.dart` around lines 295 - 332, The StreamController created in the "StreamBuilder with treatment handles stream events" test (variable controller of type StreamController<String>) may remain open if the test errors; add a test teardown to always close it by calling addTearDown(() => controller.close()) immediately after the controller is created so the controller is closed on test completion regardless of failures.
12-25: Consider adding tearDown for resource cleanup.The
setUpcreatessdkandcontextinstances, but there's no correspondingtearDownto close or dispose of these resources after each test. Whilst Dart's garbage collector will eventually reclaim them, explicitly cleaning up ensures tests don't leak state or connections.💡 Suggested addition
final contextConfig = ContextConfig.create()..setUnit('user_id', '12345'); context = sdk.createContext(contextConfig); }); + tearDown(() async { + await context.close(); + }); + group('3.1 Dart VM Behavior', () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/platform_test.dart` around lines 12 - 25, Add a tearDown block to explicitly clean up the created resources: after setUp that creates sdk (ABSmartly) and context (from ContextConfig), call their respective cleanup methods (e.g., sdk.close() or sdk.dispose(), and context.close() or context.dispose()) if those methods exist, and then null out sdk and context to avoid leaking state between tests; implement the tearDown using the same test scope so it runs after each test.
393-426: Created contexts are not disposed after use.The test creates 5
Contextinstances in the loop but never closes them. Consider usingaddTearDownto ensure all contexts are properly cleaned up after the test completes.💡 Suggested addition for cleanup
testWidgets('concurrent widget operations are safe', (tester) async { final contexts = <Context>[]; for (int i = 0; i < 5; i++) { final contextConfig = ContextConfig.create() ..setUnit('user_id', 'user_$i'); contexts.add(sdk.createContext(contextConfig)); } + addTearDown(() async { + for (final ctx in contexts) { + await ctx.close(); + } + }); await tester.pumpWidget(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/platform_test.dart` around lines 393 - 426, The test creates multiple Context objects via ContextConfig.create() and sdk.createContext(...) but never disposes them; add an addTearDown block that iterates the local contexts list and calls the Context cleanup method (e.g., context.close() or context.dispose()—use whatever the Context API provides) on each item to ensure all Context instances created by sdk.createContext are properly cleaned up after the test.packages/dart_sdk/lib/src/ab_smartly.dart (1)
54-59: Prefer immutable dependency fieldsAt Line 54–59, these injected dependencies appear write-once. Marking them
finalwill tighten invariants and reduce accidental mutation risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/lib/src/ab_smartly.dart` around lines 54 - 59, The injected dependency fields client_, contextDataProvider_, contextEventHandler_, contextEventLogger?, variableParser_, and audienceDeserializer_ should be made immutable by declaring them final; update their declarations from mutable fields to final fields (preserving the nullable marker on contextEventLogger if needed) and ensure the class constructor initializes each final field (or use required constructor params) so the code still compiles and retains the same runtime behavior.packages/dart_sdk/test/context_test.dart (1)
2103-2149: Consider consolidating duplicate test cases.
publishDoesNotClearQueueOnFailureandpublishKeepsQueueOnFailuretest the same behaviour (queue preservation on publish failure) with only minor setup differences (1 vs 2 tracked goals). Consider consolidating into a single parameterised test or differentiating them more clearly to justify both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/context_test.dart` around lines 2103 - 2149, Two tests (publishDoesNotClearQueueOnFailure and publishKeepsQueueOnFailure) duplicate the same failure behavior; consolidate them by turning them into a single parameterized test that runs the same assertions for different starting pending counts (e.g., 1 and 2 tracked events). Update the test to create a context via createReadyContext(), track N goals (use a loop), stub eventHandler.publish to return createErrorVoidCompleter(...), call expectLater(context.publish(), throwsException) and assert context.getPendingCount() equals the original pending count; remove the redundant test or replace both with this single parameterized test referencing the existing test names, context.publish, createReadyContext, context.track, eventHandler.publish, createErrorVoidCompleter, and getPendingCount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dart_sdk/lib/src/context.dart`:
- Around line 329-330: The 'number' branch currently returns
num.parse(field.value) which can throw a FormatException for malformed values;
update the handling in the switch branch (case 'number') inside the relevant
parsing routine in packages/dart_sdk/lib/src/context.dart to defensively parse
the value (e.g., use num.tryParse or wrap num.parse in a try/catch) and return a
sensible fallback (null or a default) or propagate a controlled error instead of
allowing an unhandled exception; ensure you reference and update the exact case
'number' branch where num.parse(field.value) is called.
In `@packages/dart_sdk/lib/src/json/experiment.dart`:
- Around line 121-128: The Experiment JSON round-trip is lossy because
customFieldValues is parsed in Experiment.fromMap but not emitted in
Experiment.toMap; update the toMap method (the one paired with
Experiment.fromMap) to include the customFieldValues key, serializing it as a
List by mapping each CustomFieldValue via its toMap (or equivalent) and
preserving null when customFieldValues is null; ensure the same naming/key
("customFieldValues") and type (List<Map<String, dynamic>> or null) are used and
that CustomFieldValue.toMap is invoked for each element to maintain full
round-trip fidelity.
- Around line 13-18: The factory CustomFieldValue.fromMap directly assigns
dynamic map values to non-nullable String fields causing runtime type errors;
update fromMap to explicitly validate and cast each field (name, value, type)
from data using something like data['name'] as String? or converting via
toString() with null checks, and throw a descriptive FormatException if a
required field is missing or not convertible; ensure the factory uses the
CustomFieldValue constructor only after confirmed non-null String values for
name, value, and type so parsing is safe and errors are clear.
In `@packages/flutter_sdk/lib/src/widgets/absmartly_provider.dart`:
- Around line 86-128: The provider only initializes SDK once in initState, so
updates to incoming props (widget.sdk, widget.context,
widget.defaultLoadingBehavior, widget.readyTimeout, widget._endpoint,
widget._apiKey, widget._environment, widget._application, widget._units) leave
_data stale; implement didUpdateWidget to detect changes to those inputs and
reconcile state: if supplied sdk/context changed, swap to the new ones
(disposing previously internally created SDK/context if _isInternallyCreated),
or if creation inputs changed (endpoint/apiKey/environment/application/units)
re-run _initializeSDK to recreate client/sdk/context; after any change call
_updateData and setState to update consumers, and ensure _isInternallyCreated is
maintained correctly when switching between provided vs internally created
instances.
In `@packages/flutter_sdk/lib/src/widgets/treatment.dart`:
- Around line 55-107: When reloading (in didUpdateWidget/_initializeTreatment)
reset treatment state and guard against stale async completions: set _isReady,
_timedOut, and _variant to their initial values whenever you cancel/start a new
initialization (references: didUpdateWidget, _initializeTreatment,
_cancelTimeout, _startTimeoutIfNeeded, _isReady, _timedOut, _variant). Also
capture a local token (for example the current Context instance from the
_context getter or a monotonic counter) before calling ctx.waitUntilReady() and
check that token inside the then/catch handlers so results from older
waitUntilReady() calls are ignored (references: _context, waitUntilReady). This
ensures provider replacements (ABSmartlyProvider.maybeOf/ABSmartlyProvider.of
with listen:false) or name/context changes do not let stale async work overwrite
the fresh state.
In `@packages/flutter_sdk/test/fix_verification_test.dart`:
- Around line 169-192: The tests create Context instances via Context.create
(assigned to ctx) and never close them, which can leak timers/futures; after
awaiting ctx.waitUntilReady() in both tests (the one validating units map
contents and the one checking immutability) ensure the context is closed by
calling ctx.close() (or register addTearDown(() => ctx.close())) so the Context
is properly disposed; locate the ctx variable in both tests and add the cleanup
immediately after waitUntilReady() or via addTearDown to avoid async leakage.
In `@packages/flutter_sdk/test/platform_test.dart`:
- Around line 227-238: The test currently only checks futures are non-null but
should assert they are the same object; update the assertions in the test that
calls testContext.waitUntilReady() (variables future1 and future2) to verify
identity (e.g., use the test matcher same(future2) or an identical check)
instead of isNotNull so the test validates that waitUntilReady returns the same
Future instance when called multiple times.
---
Outside diff comments:
In `@packages/dart_sdk/test/context_test.dart`:
- Around line 1750-1752: The test currently uses the synchronous exception
pattern expect(() => context.refresh(), ...) but refresh() returns a Future;
change it to the async Future pattern by asserting the Future throws, e.g. use
expect(context.refresh(), throwsA(isA<Exception>())) or await
expectLater(context.refresh(), throwsA(isA<Exception>())), keeping the
subsequent verify(dataProvider.getContextData()).called(1); reference the
context.refresh() call and the verify(...) assertion when making the change.
- Around line 1726-1728: The test is using a synchronous exception assertion for
an async function; replace expect(() => context.publish(),
throwsA(isA<Exception>())) with an asynchronous assertion like await
expectLater(context.publish(), throwsA(isA<Exception>())) so the Future returned
by context.publish() is awaited and exceptions are caught correctly; keep the
subsequent verify(eventHandler.publish(any, any)).called(1) assertion as is.
---
Duplicate comments:
In `@packages/dart_sdk/lib/src/json/experiment.dart`:
- Around line 72-75: operator== compares list contents using
ListEquality/DeepCollectionEquality but hashCode still uses the lists' instance
hashCodes; update Experiment.hashCode to compute hashes from list contents (use
the same equality type you used in operator==, e.g. const
ListEquality().hash(...) or const DeepCollectionEquality().hash(...) for nested
lists) for the fields split, trafficSplit, applications, and variants so
equality/hashCode are consistent; ensure the collection equality class you pick
matches the one used in operator== and call its .hash(...) for each list when
composing the overall hash.
In `@packages/flutter_sdk/lib/src/widgets/treatment_builder.dart`:
- Around line 64-117: The widget fails to reset local state when reinitialising
and allows stale async completions to overwrite newer state, and it doesn't
observe provider swaps because _context and _providerData use listen: false; fix
by making _context/_providerData listen to provider changes (use listen: true or
rely on context.watch semantics) so ambient provider swaps trigger
didUpdateWidget, ensure didUpdateWidget and initState clear/reset local flags
(_isReady, _variant, _variables) before calling _initializeTreatment, and
protect async completions in _initializeTreatment/_updateTreatment by capturing
a unique token or the ctx instance (final token/ctx = _context) and verifying it
still matches current _context (or token) and mounted before applying setState;
also keep using _startTimeoutIfNeeded/_cancelTimeout but cancel and reset
timeouts when reinitialising.
In `@packages/flutter_sdk/lib/src/widgets/variable_value.dart`:
- Around line 66-113: Reset readiness/timed-out state and guard async
completions with a generation token when reinitializing: in didUpdateWidget(...)
(and any reinit path in _initializeValue) clear _isReady and _timedOut, call
_cancelTimeout(), increment a private int token (e.g., _initGeneration) and
capture its value in _initializeValue before any async wait; after
ctx.waitUntilReady().then(...) or catchError(...) only apply state changes if
mounted and the captured token still equals _initGeneration to avoid stale
completions; also change the _context getter and _providerData lookup to use the
provider with listen: true (e.g., ABSmartlyProvider.of(context, listen: true) /
maybeOf(context, listen: true)) so ambient provider swaps trigger widget
updates.
---
Nitpick comments:
In `@packages/dart_sdk/lib/src/ab_smartly.dart`:
- Around line 54-59: The injected dependency fields client_,
contextDataProvider_, contextEventHandler_, contextEventLogger?,
variableParser_, and audienceDeserializer_ should be made immutable by declaring
them final; update their declarations from mutable fields to final fields
(preserving the nullable marker on contextEventLogger if needed) and ensure the
class constructor initializes each final field (or use required constructor
params) so the code still compiles and retains the same runtime behavior.
In `@packages/dart_sdk/test/context_test.dart`:
- Around line 2103-2149: Two tests (publishDoesNotClearQueueOnFailure and
publishKeepsQueueOnFailure) duplicate the same failure behavior; consolidate
them by turning them into a single parameterized test that runs the same
assertions for different starting pending counts (e.g., 1 and 2 tracked events).
Update the test to create a context via createReadyContext(), track N goals (use
a loop), stub eventHandler.publish to return createErrorVoidCompleter(...), call
expectLater(context.publish(), throwsException) and assert
context.getPendingCount() equals the original pending count; remove the
redundant test or replace both with this single parameterized test referencing
the existing test names, context.publish, createReadyContext, context.track,
eventHandler.publish, createErrorVoidCompleter, and getPendingCount.
In `@packages/flutter_sdk/test/platform_test.dart`:
- Around line 295-332: The StreamController created in the "StreamBuilder with
treatment handles stream events" test (variable controller of type
StreamController<String>) may remain open if the test errors; add a test
teardown to always close it by calling addTearDown(() => controller.close())
immediately after the controller is created so the controller is closed on test
completion regardless of failures.
- Around line 12-25: Add a tearDown block to explicitly clean up the created
resources: after setUp that creates sdk (ABSmartly) and context (from
ContextConfig), call their respective cleanup methods (e.g., sdk.close() or
sdk.dispose(), and context.close() or context.dispose()) if those methods exist,
and then null out sdk and context to avoid leaking state between tests;
implement the tearDown using the same test scope so it runs after each test.
- Around line 393-426: The test creates multiple Context objects via
ContextConfig.create() and sdk.createContext(...) but never disposes them; add
an addTearDown block that iterates the local contexts list and calls the Context
cleanup method (e.g., context.close() or context.dispose()—use whatever the
Context API provides) on each item to ensure all Context instances created by
sdk.createContext are properly cleaned up after the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92477e78-5657-4d1a-a2ef-bf35a630a22b
📒 Files selected for processing (24)
packages/dart_sdk/lib/src/ab_smartly.dartpackages/dart_sdk/lib/src/client_config.dartpackages/dart_sdk/lib/src/context.dartpackages/dart_sdk/lib/src/context_config.dartpackages/dart_sdk/lib/src/default_http_client.dartpackages/dart_sdk/lib/src/http_client.dartpackages/dart_sdk/lib/src/json/experiment.dartpackages/dart_sdk/lib/src/jsonexpr/json_expr.dartpackages/dart_sdk/lib/src/jsonexpr/operators/greater_than_operator.dartpackages/dart_sdk/lib/src/jsonexpr/operators/greater_than_or_equal_operator.dartpackages/dart_sdk/lib/src/jsonexpr/operators/less_than_operator.dartpackages/dart_sdk/lib/src/jsonexpr/operators/less_than_or_equal_operator.dartpackages/dart_sdk/test/ab_smartly_test.dartpackages/dart_sdk/test/client_config_test.dartpackages/dart_sdk/test/context_test.dartpackages/dart_sdk/test/jsonexpr/operator_test.dartpackages/flutter_sdk/lib/src/widgets/absmartly_provider.dartpackages/flutter_sdk/lib/src/widgets/treatment.dartpackages/flutter_sdk/lib/src/widgets/treatment_builder.dartpackages/flutter_sdk/lib/src/widgets/treatment_switch.dartpackages/flutter_sdk/lib/src/widgets/variable_value.dartpackages/flutter_sdk/test/fix_verification_test.dartpackages/flutter_sdk/test/performance_test.dartpackages/flutter_sdk/test/platform_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/flutter_sdk/test/performance_test.dart
Comment on lines
+86
to
+128
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| _initializeSDK(); | ||
| } | ||
|
|
||
| void _initializeSDK() { | ||
| if (widget.sdk != null && widget.context != null) { | ||
| _sdk = widget.sdk!; | ||
| _context = widget.context!; | ||
| _isInternallyCreated = false; | ||
| } else { | ||
| _isInternallyCreated = true; | ||
|
|
||
| final clientConfig = ClientConfig.create( | ||
| endpoint: widget._endpoint!, | ||
| apiKey: widget._apiKey!, | ||
| environment: widget._environment!, | ||
| application: widget._application!, | ||
| ); | ||
|
|
||
| final client = Client.create(clientConfig); | ||
|
|
||
| final sdkConfig = ABSmartlyConfig.create(client: client); | ||
|
|
||
| _sdk = ABSmartly(sdkConfig); | ||
|
|
||
| final contextConfig = ContextConfig.create(units: widget._units!); | ||
|
|
||
| _context = _sdk.createContext(contextConfig); | ||
| } | ||
| _updateData(); | ||
| } | ||
|
|
||
| void _updateData() { | ||
| _data = ABSmartlyData( | ||
| sdk: _sdk, | ||
| context: _context, | ||
| defaultLoadingBehavior: widget.defaultLoadingBehavior, | ||
| readyTimeout: widget.readyTimeout, | ||
| resetContext: _resetContext, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Provider state does not react to widget updates
_initializeSDK() only runs in initState (Line 87–90). If parent props change (e.g. supplied sdk/context, defaultLoadingBehavior, readyTimeout, or .create() inputs), _data can become stale because there is no didUpdateWidget reconciliation.
Suggested fix sketch
+ `@override`
+ void didUpdateWidget(covariant ABSmartlyProvider oldWidget) {
+ super.didUpdateWidget(oldWidget);
+
+ final sourceChanged =
+ oldWidget.sdk != widget.sdk ||
+ oldWidget.context != widget.context ||
+ oldWidget._endpoint != widget._endpoint ||
+ oldWidget._apiKey != widget._apiKey ||
+ oldWidget._environment != widget._environment ||
+ oldWidget._application != widget._application ||
+ oldWidget._units != widget._units;
+
+ final loadingConfigChanged =
+ oldWidget.defaultLoadingBehavior != widget.defaultLoadingBehavior ||
+ oldWidget.readyTimeout != widget.readyTimeout;
+
+ if (sourceChanged) {
+ _initializeSDK();
+ } else if (loadingConfigChanged) {
+ _updateData();
+ }
+ }Also applies to: 154-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/lib/src/widgets/absmartly_provider.dart` around lines 86
- 128, The provider only initializes SDK once in initState, so updates to
incoming props (widget.sdk, widget.context, widget.defaultLoadingBehavior,
widget.readyTimeout, widget._endpoint, widget._apiKey, widget._environment,
widget._application, widget._units) leave _data stale; implement didUpdateWidget
to detect changes to those inputs and reconcile state: if supplied sdk/context
changed, swap to the new ones (disposing previously internally created
SDK/context if _isInternallyCreated), or if creation inputs changed
(endpoint/apiKey/environment/application/units) re-run _initializeSDK to
recreate client/sdk/context; after any change call _updateData and setState to
update consumers, and ensure _isInternallyCreated is maintained correctly when
switching between provided vs internally created instances.
Comment on lines
+55
to
+107
| Context get _context { | ||
| if (widget.context != null) { | ||
| return widget.context!; | ||
| } | ||
| return ABSmartlyProvider.of(context, listen: false).context; | ||
| } | ||
|
|
||
| ABSmartlyData? get _providerData { | ||
| return ABSmartlyProvider.maybeOf(context, listen: false); | ||
| } | ||
|
|
||
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| _initializeTreatment(); | ||
| } | ||
|
|
||
| @override | ||
| void didUpdateWidget(Treatment oldWidget) { | ||
| super.didUpdateWidget(oldWidget); | ||
| if (oldWidget.name != widget.name || oldWidget.context != widget.context) { | ||
| _cancelTimeout(); | ||
| _initializeTreatment(); | ||
| } | ||
| } | ||
|
|
||
| void _initializeTreatment() { | ||
| final ctx = _context; | ||
|
|
||
| if (ctx.isReady()) { | ||
| _updateTreatment(ctx); | ||
| } else { | ||
| _startTimeoutIfNeeded(); | ||
| ctx.waitUntilReady().then((_) { | ||
| if (mounted) { | ||
| _cancelTimeout(); | ||
| _updateTreatment(ctx); | ||
| } | ||
| }).catchError((error) { | ||
| assert(() { | ||
| debugPrint('ABSmartly: Treatment "${widget.name}" error: $error'); | ||
| return true; | ||
| }()); | ||
| if (mounted) { | ||
| _cancelTimeout(); | ||
| setState(() { | ||
| _isReady = true; | ||
| _variant = 0; | ||
| }); | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
State reload is not isolated from stale async work
At Line 75–78, reload does not reset _isReady/_timedOut/_variant, and Line 88–105 can still commit results from an older waitUntilReady() call. Combined with listen: false at Line 59/63, provider context replacements can be missed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/lib/src/widgets/treatment.dart` around lines 55 - 107,
When reloading (in didUpdateWidget/_initializeTreatment) reset treatment state
and guard against stale async completions: set _isReady, _timedOut, and _variant
to their initial values whenever you cancel/start a new initialization
(references: didUpdateWidget, _initializeTreatment, _cancelTimeout,
_startTimeoutIfNeeded, _isReady, _timedOut, _variant). Also capture a local
token (for example the current Context instance from the _context getter or a
monotonic counter) before calling ctx.waitUntilReady() and check that token
inside the then/catch handlers so results from older waitUntilReady() calls are
ignored (references: _context, waitUntilReady). This ensures provider
replacements (ABSmartlyProvider.maybeOf/ABSmartlyProvider.of with listen:false)
or name/context changes do not let stale async work overwrite the fresh state.
Comment on lines
+227
to
+238
| test('context waitUntilReady returns same future when called multiple times', | ||
| () async { | ||
| final contextConfig = ContextConfig.create() | ||
| ..setUnit('user_id', '12345'); | ||
| final testContext = sdk.createContext(contextConfig); | ||
|
|
||
| final future1 = testContext.waitUntilReady(); | ||
| final future2 = testContext.waitUntilReady(); | ||
|
|
||
| expect(future1, isNotNull); | ||
| expect(future2, isNotNull); | ||
| }); |
There was a problem hiding this comment.
Test assertion does not match the test name.
The test is named "context waitUntilReady returns same future when called multiple times", implying it should verify that the returned futures are identical (the same object). However, the assertions only check that both futures are not null, which doesn't validate the stated behaviour.
🔧 Proposed fix to verify future identity
final future1 = testContext.waitUntilReady();
final future2 = testContext.waitUntilReady();
- expect(future1, isNotNull);
- expect(future2, isNotNull);
+ expect(identical(future1, future2), isTrue,
+ reason: 'waitUntilReady should return the same future instance');📝 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
| test('context waitUntilReady returns same future when called multiple times', | |
| () async { | |
| final contextConfig = ContextConfig.create() | |
| ..setUnit('user_id', '12345'); | |
| final testContext = sdk.createContext(contextConfig); | |
| final future1 = testContext.waitUntilReady(); | |
| final future2 = testContext.waitUntilReady(); | |
| expect(future1, isNotNull); | |
| expect(future2, isNotNull); | |
| }); | |
| test('context waitUntilReady returns same future when called multiple times', | |
| () async { | |
| final contextConfig = ContextConfig.create() | |
| ..setUnit('user_id', '12345'); | |
| final testContext = sdk.createContext(contextConfig); | |
| final future1 = testContext.waitUntilReady(); | |
| final future2 = testContext.waitUntilReady(); | |
| expect(identical(future1, future2), isTrue, | |
| reason: 'waitUntilReady should return the same future instance'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/test/platform_test.dart` around lines 227 - 238, The
test currently only checks futures are non-null but should assert they are the
same object; update the assertions in the test that calls
testContext.waitUntilReady() (variables future1 and future2) to verify identity
(e.g., use the test matcher same(future2) or an identical check) instead of
isNotNull so the test validates that waitUntilReady returns the same Future
instance when called multiple times.
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)
packages/dart_sdk/lib/src/context.dart (1)
506-546:⚠️ Potential issue | 🔴 CriticalPrevent overlapping
flush()publishes; current logic can duplicate sends and throw onremoveRange.If
flush()is called again before the first publish resolves, both calls snapshot pending events and both later executeremoveRange(0, snapshotLength). That can publish duplicates and can raise aRangeErrorwhen the second completion removes already-removed items.🐛 Proposed fix (single in-flight publish guard)
Future<void> flush() async { clearTimeout(); + if (publishingFuture_ != null) { + return publishingFuture_!; + } if (!failed_) { if (pendingCount_ > 0) { @@ final Completer<void> result = Completer<void>(); + publishingFuture_ = result.future; eventHandler_.publish(this, event).future.then((_) { exposures_.removeRange(0, exposures.length); achievements_.removeRange(0, achievements.length); pendingCount_ -= eventCount; logEvent(EventType.publish, event); result.complete(); }).catchError((error) { logError(error); result.completeError(error); + }).whenComplete(() { + publishingFuture_ = null; }); return result.future; } @@ Timer? timeout_; Timer? refreshTimer_; + Future<void>? publishingFuture_; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/lib/src/context.dart` around lines 506 - 546, The flush() method can run concurrently and cause duplicate publishes and RangeErrors when exposures_.removeRange/achievements_.removeRange are called; add a single in-flight publish guard (e.g., a private bool publishing_ or a Completer<void> inFlightPublish_) checked at the start of flush() so if a publish is already pending the method returns the existing in-flight Future, and when starting a new publish set the guard and clear it in both the then and catchError handlers; ensure you still snapshot exposures_, achievements_, pendingCount_, and units_ before calling eventHandler_.publish and only mutate exposures_/achievements_/pendingCount_ after that single publish completes to avoid overlapping removals.
♻️ Duplicate comments (1)
packages/dart_sdk/lib/src/context.dart (1)
329-330:⚠️ Potential issue | 🟡 MinorUse defensive numeric parsing for custom fields.
Line 330 uses
num.parse(field.value), which throws on malformed payloads and can crash this accessor path.🛡️ Proposed fix
case 'number': - return num.parse(field.value); + return num.tryParse(field.value);#!/bin/bash # Verify all custom-field numeric parsing paths and identify throwing parses. rg -nP --type=dart "case 'number':" -A5 -B2 rg -nP --type=dart '\bnum\.parse\s*\(' rg -nP --type=dart '\bnum\.tryParse\s*\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/lib/src/context.dart` around lines 329 - 330, The numeric branch using num.parse(field.value) in the case 'number' path is unsafe; replace the direct parse with num.tryParse(field.value) and handle the null result gracefully (e.g., return null, a default value, or throw a descriptive error) so malformed payloads don't crash the accessor; update the switch branch around case 'number' (the code containing num.parse(field.value)) to perform tryParse and explicit null-handling/logging instead of calling num.parse directly.
🧹 Nitpick comments (2)
packages/dart_sdk/test/ab_smartly_test.dart (1)
9-13: PreferContextPublisherin tests instead of deprecatedContextEventHandler.The tests still mock and type against the deprecated alias. Migrating now keeps test code aligned with the new public API and avoids deprecation drift.
Also applies to: 28-29, 109-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/ab_smartly_test.dart` around lines 9 - 13, Replace usages of the deprecated ContextEventHandler in this test with the new ContextPublisher: update the import (replace 'context_event_handler.dart' with 'context_publisher.dart' if present), change all type annotations, mocks, and variables named or typed as ContextEventHandler to ContextPublisher, and update any mock setup/verify calls to reference ContextPublisher methods; ensure classes like DefaultContextEventHandler or DefaultContextPublisher references are aligned (use DefaultContextPublisher where appropriate) so the test compiles against the new public API.packages/dart_sdk/lib/src/default_context_publisher.dart (1)
14-15: Add explicitContextparameter type in the override.Line 14 currently uses an untyped
contextparameter. TheContextPublisherinterface requires the parameter to be typed asContext, so the implementation should match for consistency and static type safety.♻️ Proposed refactor
import 'dart:async'; import 'json/publish_event.dart'; +import 'context.dart'; import 'context_publisher.dart'; import 'client.dart'; @@ - Completer<void> publish(context, PublishEvent event) { + Completer<void> publish(Context context, PublishEvent event) { return client_.publish(event); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/lib/src/default_context_publisher.dart` around lines 14 - 15, The override of publish in DefaultContextPublisher uses an untyped parameter; change its signature to match the ContextPublisher interface by typing the first parameter as Context (i.e., publish(Context context, PublishEvent event)), and ensure the method body still returns client_.publish(event); update any related imports if needed so Context is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/dart_sdk/lib/src/context.dart`:
- Around line 506-546: The flush() method can run concurrently and cause
duplicate publishes and RangeErrors when
exposures_.removeRange/achievements_.removeRange are called; add a single
in-flight publish guard (e.g., a private bool publishing_ or a Completer<void>
inFlightPublish_) checked at the start of flush() so if a publish is already
pending the method returns the existing in-flight Future, and when starting a
new publish set the guard and clear it in both the then and catchError handlers;
ensure you still snapshot exposures_, achievements_, pendingCount_, and units_
before calling eventHandler_.publish and only mutate
exposures_/achievements_/pendingCount_ after that single publish completes to
avoid overlapping removals.
---
Duplicate comments:
In `@packages/dart_sdk/lib/src/context.dart`:
- Around line 329-330: The numeric branch using num.parse(field.value) in the
case 'number' path is unsafe; replace the direct parse with
num.tryParse(field.value) and handle the null result gracefully (e.g., return
null, a default value, or throw a descriptive error) so malformed payloads don't
crash the accessor; update the switch branch around case 'number' (the code
containing num.parse(field.value)) to perform tryParse and explicit
null-handling/logging instead of calling num.parse directly.
---
Nitpick comments:
In `@packages/dart_sdk/lib/src/default_context_publisher.dart`:
- Around line 14-15: The override of publish in DefaultContextPublisher uses an
untyped parameter; change its signature to match the ContextPublisher interface
by typing the first parameter as Context (i.e., publish(Context context,
PublishEvent event)), and ensure the method body still returns
client_.publish(event); update any related imports if needed so Context is
resolved.
In `@packages/dart_sdk/test/ab_smartly_test.dart`:
- Around line 9-13: Replace usages of the deprecated ContextEventHandler in this
test with the new ContextPublisher: update the import (replace
'context_event_handler.dart' with 'context_publisher.dart' if present), change
all type annotations, mocks, and variables named or typed as ContextEventHandler
to ContextPublisher, and update any mock setup/verify calls to reference
ContextPublisher methods; ensure classes like DefaultContextEventHandler or
DefaultContextPublisher references are aligned (use DefaultContextPublisher
where appropriate) so the test compiles against the new public API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e4e0a30-ace6-486b-9c60-1cb0bda736f9
📒 Files selected for processing (9)
packages/dart_sdk/lib/src/ab_smartly.dartpackages/dart_sdk/lib/src/absmartly_sdk.dartpackages/dart_sdk/lib/src/absmartly_sdk_config.dartpackages/dart_sdk/lib/src/context.dartpackages/dart_sdk/lib/src/context_event_handler.dartpackages/dart_sdk/lib/src/context_publisher.dartpackages/dart_sdk/lib/src/default_context_event_handler.dartpackages/dart_sdk/lib/src/default_context_publisher.dartpackages/dart_sdk/test/ab_smartly_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dart_sdk/lib/src/ab_smartly.dart
…, null safety, and test accuracy
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dart_sdk/lib/src/json/experiment.dart (1)
70-109:⚠️ Potential issue | 🟠 MajorInclude
customFieldValuesinExperimentvalue semantics.
Experiment.customFieldValues(Line 48) is now part of model state and JSON round-trip, but it is omitted fromoperator ==andhashCode(Lines 70-109). Two instances that differ only on custom fields currently compare equal.💡 Proposed fix
class CustomFieldValue { final String name; final String value; final String type; @@ Map<String, dynamic> toMap() { return { 'name': name, 'value': value, 'type': type, }; } + + `@override` + bool operator ==(Object other) => + identical(this, other) || + other is CustomFieldValue && + other.name == name && + other.value == value && + other.type == type; + + `@override` + int get hashCode => Object.hash(name, value, type); } @@ const ListEquality().equals(split, other.split) && const ListEquality().equals(trafficSplit, other.trafficSplit) && const ListEquality().equals(applications, other.applications) && const ListEquality().equals(variants, other.variants) && + const ListEquality<CustomFieldValue>() + .equals(customFieldValues, other.customFieldValues) && audienceStrict == other.audienceStrict && audience == other.audience; @@ listEquality.hash(split), listEquality.hash(trafficSplit), listEquality.hash(applications), listEquality.hash(variants), + const ListEquality<CustomFieldValue>().hash(customFieldValues), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/lib/src/json/experiment.dart` around lines 70 - 109, The Experiment class's value semantics omit the customFieldValues field: update the operator == and hashCode implementations to include customFieldValues so instances that differ only in customFieldValues are distinguished; in operator == add a comparison using const ListEquality().equals(customFieldValues, other.customFieldValues) (or appropriate MapEquality/DeepCollectionEquality if customFieldValues is a map), and in hashCode include listEquality.hash(customFieldValues) (or the corresponding equality.hash(customFieldValues)) alongside the other fields referenced in Object.hash, ensuring you use the same equality helper (e.g., ListEquality or MapEquality) used for the other collection fields.
♻️ Duplicate comments (6)
packages/flutter_sdk/test/platform_test.dart (1)
227-239:⚠️ Potential issue | 🟡 MinorAssert Future identity directly, not resolved value identity.
This test still doesn’t validate “returns same future”.
result1/result2can be equal even when futures differ.Suggested fix
final future1 = testContext.waitUntilReady(); final future2 = testContext.waitUntilReady(); + expect(future1, same(future2)); final result1 = await future1; final result2 = await future2; expect(result1, same(result2));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/test/platform_test.dart` around lines 227 - 239, The test currently awaits both futures and compares their resolved values, which doesn't prove the same Future instance is returned; instead assert the futures themselves are identical: after obtaining future1 and future2 from testContext.waitUntilReady(), use expect(future1, same(future2)) (or an equivalent identity check) to verify waitUntilReady returns the same Future instance; locate this in the test named "context waitUntilReady returns same future when called multiple times" using symbols testContext and waitUntilReady.packages/flutter_sdk/lib/src/widgets/treatment_switch.dart (1)
99-155:⚠️ Potential issue | 🟠 Major
TreatmentSwitchcan still commit stale variant state from old async work.Same pattern as the other widgets: no generation token around
waitUntilReady()completion and no robust ambient provider-change tracking. This can pin the switch to an outdated variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/lib/src/widgets/treatment_switch.dart` around lines 99 - 155, The _initializeTreatment flow in TreatmentSwitch can apply stale async results because the waitUntilReady() completion isn't tied to a generation token and doesn't verify the ambient provider hasn't changed; update _initializeTreatment (and didUpdateWidget) to use a local generation/version token (e.g., increment a private int _generation in didUpdateWidget and capture it inside _initializeTreatment) and, after waitUntilReady() or any async callback, verify the captured token matches the current _generation and that mounted is true before calling _cancelTimeout(), setState(), or _updateTreatment(ctx); also ensure _context resolution still matches the captured provider by reading ctx into a local variable and comparing provider identity if necessary to avoid applying results from a stale provider.packages/flutter_sdk/lib/src/widgets/treatment.dart (1)
55-110:⚠️ Potential issue | 🟠 MajorStale async completions can overwrite state after context/provider changes.
waitUntilReady()callbacks are not version-guarded, and provider swaps are not observed (listen: falseeverywhere). An older context can still commit_variantafter a reload.Suggested hardening pattern
class _TreatmentState extends State<Treatment> { + int _reloadVersion = 0; + Context? _observedProviderContext; bool _isReady = false; bool _timedOut = false; int _variant = 0; Timer? _timeoutTimer; + `@override` + void didChangeDependencies() { + super.didChangeDependencies(); + if (widget.context == null) { + final providerCtx = ABSmartlyProvider.of(context).context; + if (!identical(providerCtx, _observedProviderContext)) { + _observedProviderContext = providerCtx; + _reload(); + } + } + } + + void _reload() { + _cancelTimeout(); + _reloadVersion++; + setState(() { + _isReady = false; + _timedOut = false; + _variant = 0; + }); + _initializeTreatment(); + } + void _initializeTreatment() { final ctx = _context; + final version = _reloadVersion; ... - ctx.waitUntilReady().then((_) { - if (mounted) { + ctx.waitUntilReady().then((_) { + if (mounted && version == _reloadVersion && identical(ctx, _context)) { _cancelTimeout(); _updateTreatment(ctx); } }).catchError((error) { ... - if (mounted) { + if (mounted && version == _reloadVersion && identical(ctx, _context)) {packages/flutter_sdk/lib/src/widgets/variable_value.dart (1)
66-118:⚠️ Potential issue | 🟠 Major
VariableValuestill has stale async and provider-swap race exposure.A previous
waitUntilReady()completion can still write_valueafter a reload; ambient provider context changes are also not explicitly tracked.Please apply the same fix pattern as
Treatment: dependency-driven provider change detection (didChangeDependencies) plus a reload/version token check inthen/catchError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/lib/src/widgets/variable_value.dart` around lines 66 - 118, Add dependency-change detection and a reload/version token to prevent stale async writes: introduce a private token field (e.g., _reloadToken) incremented whenever the provider/context can change (in didUpdateWidget and a new didChangeDependencies override) and when resetting state (_cancelTimeout, set _isReady/_timedOut/_value). In _initializeValue capture both the current ctx (from _context) and the current token into local finals, and in the futures' then and catchError blocks verify the captured token still equals the instance token and the captured ctx equals _context before calling _cancelTimeout, _updateValue, or setState. This uses existing symbols: _context getter, _initializeValue, didUpdateWidget, didChangeDependencies (new), _updateValue, _cancelTimeout, _startTimeoutIfNeeded, and the state fields _isReady/_timedOut/_value to safely ignore stale completions.packages/flutter_sdk/pubspec.yaml (2)
11-15:⚠️ Potential issue | 🟠 MajorLocal
path:dependency still blocks pub.dev publishing.The
absmartly_dartdependency uses a local path (../dart_sdk), which pub.dev rejects with aninvalid_dependencyerror. If this package is intended for publishing, either:
- Replace with a hosted pub.dev dependency or git source, or
- Add
publish_to: noneat the top of the file to mark it as private.This issue was raised in the previous review and remains unresolved.
🔧 Option A: Mark as non-publishable (if local-only)
name: absmartly_sdk +publish_to: none description: ABsmartly A/B Testing SDK for Flutter - Flutter wrapper with widgets and providers🔧 Option B: Use git dependency (if publishing is intended later)
dependencies: flutter: sdk: flutter - absmartly_dart: - path: ../dart_sdk + absmartly_dart: + git: + url: https://github.com/absmartly/flutter-sdk.git + path: packages/dart_sdk🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/pubspec.yaml` around lines 11 - 15, The pubspec currently uses a local path dependency "absmartly_dart: path: ../dart_sdk" which blocks publishing; either replace that entry with a hosted or git dependency (e.g., change absmartly_dart to a git or hosted version) if you intend to publish, or mark the package private by adding publish_to: none at the top of pubspec.yaml; update the dependency declaration or add the publish_to field accordingly and ensure the dependency name "absmartly_dart" and its declaration are the only changes so tooling picks up the new source.
7-9:⚠️ Potential issue | 🟠 MajorFlutter version constraint still incompatible with Dart SDK constraint.
The constraint was updated from
>=1.17.0to>=3.3.0, but this still does not satisfy the Dart SDK requirement of>=2.18.6. Flutter 3.3.0 bundled Dart 2.18.0, whereas the earliest Flutter release with Dart 2.18.6 is Flutter 3.3.10. Update line 9 toflutter: ">=3.3.10"to ensure compatibility.🔧 Suggested fix
environment: sdk: '>=2.18.6 <4.0.0' - flutter: ">=3.3.0" + flutter: ">=3.3.10"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/pubspec.yaml` around lines 7 - 9, pubspec.yaml's environment block has a Flutter constraint (flutter: ">=3.3.0") that is incompatible with the Dart SDK constraint (sdk: '>=2.18.6'); update the Flutter constraint to ">=3.3.10" so it matches the Dart SDK requirement. Open the environment section in pubspec.yaml and change the flutter version string in the line containing flutter: ">=3.3.0" to flutter: ">=3.3.10", then save and run pub get to verify compatibility.
🧹 Nitpick comments (2)
packages/flutter_sdk/README.md (1)
126-131: Consider adding null-safety context in examples.The example uses
context.getTreatment()directly, but earlier sections (lines 59, 72) show context as nullableContext?. For consistency and clarity, consider adding a note that these examples assume a non-null context, or show the null check pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/flutter_sdk/README.md` around lines 126 - 131, The example using context.getTreatment assumes a non-null Context, but earlier signatures show Context?; update the snippet or add a short note clarifying null-safety: either show the null-check/unwrapping pattern (e.g., guard for context == null or use a local non-null variable) before calling context.getTreatment, or explicitly state "assuming context is non-null" so readers know to handle Context? in code paths that may yield null; reference the nullable type Context? and the getTreatment call when making this change.packages/dart_sdk/test/default_http_client_test.dart (1)
106-111: Strengthen POST/PUT call assertions to catch regressions earlier.Line 106 and Line 140 currently verify
anyURI andanyNamed('body'), so malformed endpoints or payloads can slip through undetected.Proposed tightening of assertions
@@ - final body = {'title': 'foo', 'body': 'bar', 'userId': 1}; + final body = {'title': 'foo', 'body': 'bar', 'userId': 1}; + final encodedBody = utf8.encode(jsonEncode(body)); final response = await client.post( 'https://example.com/api/posts', null, null, - utf8.encode(jsonEncode(body)), + encodedBody, ); @@ verify(mockHttpClient.post( - any, + argThat(predicate<Uri>((uri) => + uri.host == 'example.com' && uri.path == '/api/posts')), headers: anyNamed('headers'), - body: anyNamed('body'), + body: argThat( + predicate<Object?>((value) => + value is List<int> && utf8.decode(value) == jsonEncode(body)), + named: 'body', + ), )).called(1); @@ - final body = {'title': 'updated', 'body': 'bar', 'userId': 1}; + final body = {'title': 'updated', 'body': 'bar', 'userId': 1}; + final encodedBody = utf8.encode(jsonEncode(body)); final response = await client.put( 'https://example.com/api/posts/1', null, null, - utf8.encode(jsonEncode(body)), + encodedBody, ); @@ verify(mockHttpClient.put( - any, + argThat(predicate<Uri>((uri) => + uri.host == 'example.com' && uri.path == '/api/posts/1')), headers: anyNamed('headers'), - body: anyNamed('body'), + body: argThat( + predicate<Object?>((value) => + value is List<int> && utf8.decode(value) == jsonEncode(body)), + named: 'body', + ), )).called(1);Also applies to: 140-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dart_sdk/test/default_http_client_test.dart` around lines 106 - 111, The test currently verifies mockHttpClient.post/put with loose matchers (any and anyNamed('body')) allowing wrong endpoints or payloads; update the assertions to verify the expected URI and exact body content instead of any (e.g., replace any for the first positional arg with the concrete Uri used in the code under test and replace anyNamed('body') with a matcher or string equal to the expected JSON/body), keep headers asserted with anyNamed('headers') if headers vary or match specific headers if deterministic; apply the same tightening to the mockHttpClient.put verification (use mockHttpClient.post and mockHttpClient.put as the identifying symbols to locate the assertions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/dart_sdk/test/default_http_client_test.dart`:
- Around line 302-309: The test currently only asserts that
clientWithRetries.get throws, but must also assert that the client retried the
expected number of times; update the test to capture the thrown exception (e.g.,
using try/catch or expectLater that returns the error) and then assert the retry
count on the mock underlying client or retry counter equals the expected
attempts (e.g., verify/mockCallCount for the underlying HTTP call was invoked
maxRetries + 1 times) referencing clientWithRetries.get and the mock underlying
client or retry policy variable used in this test.
In `@packages/flutter_sdk/README.md`:
- Around line 53-77: The two examples under "Synchronously" and "Asynchronously"
are identical; update them to show the real difference: in the synchronous
example use sdk.createContext(contextConfig).waitUntilReady() (references:
ContextConfig, sdk.createContext, waitUntilReady, Context) and in the
asynchronous example show non-blocking creation (references: ContextConfig,
sdk.createContext, Context) and explain that the returned Context may not be
ready yet and should be checked with context.isReady() instead of awaiting;
remove the duplicate code and ensure each section demonstrates the correct
pattern and mentions isReady() for the async case.
In `@packages/flutter_sdk/test/error_recovery_test.dart`:
- Around line 101-127: The test "timeout fallback shows control after
readyTimeout in placeholder mode" lacks any assertion; add an expect to verify
the fallback control is displayed after the readyTimeout. After the second pump
(await tester.pump(const Duration(milliseconds: 100));) use tester's finder to
locate the Text widget with "Control After Timeout" (or verify that a widget
matching the control variant for Treatment 'test_experiment' is present) and
assert it is found (e.g., expect(found, findsOneWidget)); keep the check tied to
the ABSmartlyProvider/Treatment setup and readyTimeout/defaultLoadingBehavior
used in the test.
- Around line 273-295: The test `isFailed reflects context state` currently only
asserts capturedIsFailed is not null; change it to assert the provider value
matches the context's failure state by comparing capturedIsFailed to the created
context's actual state (e.g. replace expect(capturedIsFailed, isNotNull) with
expect(capturedIsFailed, equals(context.isFailed)) or the appropriate context
property), locating the values via
ContextConfig.create()/setUnit('user_id','12345'), sdk.createContext(...) and
ABSmartlyProvider.of(...) / capturedIsFailed.
- Around line 332-354: The test 'context waitUntilReady can be called multiple
times' currently just calls data.context.waitUntilReady() twice; change it to
capture both returned Futures (e.g., final f1 = data.context.waitUntilReady();
final f2 = data.context.waitUntilReady();) and then assert they are identical
(expect(f1, same(f2))) and/or await both to ensure the same readiness result;
update the test body where data.context.waitUntilReady() is invoked to use these
captured futures and assertions to verify idempotency/safety of waitUntilReady.
---
Outside diff comments:
In `@packages/dart_sdk/lib/src/json/experiment.dart`:
- Around line 70-109: The Experiment class's value semantics omit the
customFieldValues field: update the operator == and hashCode implementations to
include customFieldValues so instances that differ only in customFieldValues are
distinguished; in operator == add a comparison using const
ListEquality().equals(customFieldValues, other.customFieldValues) (or
appropriate MapEquality/DeepCollectionEquality if customFieldValues is a map),
and in hashCode include listEquality.hash(customFieldValues) (or the
corresponding equality.hash(customFieldValues)) alongside the other fields
referenced in Object.hash, ensuring you use the same equality helper (e.g.,
ListEquality or MapEquality) used for the other collection fields.
---
Duplicate comments:
In `@packages/flutter_sdk/lib/src/widgets/treatment_switch.dart`:
- Around line 99-155: The _initializeTreatment flow in TreatmentSwitch can apply
stale async results because the waitUntilReady() completion isn't tied to a
generation token and doesn't verify the ambient provider hasn't changed; update
_initializeTreatment (and didUpdateWidget) to use a local generation/version
token (e.g., increment a private int _generation in didUpdateWidget and capture
it inside _initializeTreatment) and, after waitUntilReady() or any async
callback, verify the captured token matches the current _generation and that
mounted is true before calling _cancelTimeout(), setState(), or
_updateTreatment(ctx); also ensure _context resolution still matches the
captured provider by reading ctx into a local variable and comparing provider
identity if necessary to avoid applying results from a stale provider.
In `@packages/flutter_sdk/lib/src/widgets/variable_value.dart`:
- Around line 66-118: Add dependency-change detection and a reload/version token
to prevent stale async writes: introduce a private token field (e.g.,
_reloadToken) incremented whenever the provider/context can change (in
didUpdateWidget and a new didChangeDependencies override) and when resetting
state (_cancelTimeout, set _isReady/_timedOut/_value). In _initializeValue
capture both the current ctx (from _context) and the current token into local
finals, and in the futures' then and catchError blocks verify the captured token
still equals the instance token and the captured ctx equals _context before
calling _cancelTimeout, _updateValue, or setState. This uses existing symbols:
_context getter, _initializeValue, didUpdateWidget, didChangeDependencies (new),
_updateValue, _cancelTimeout, _startTimeoutIfNeeded, and the state fields
_isReady/_timedOut/_value to safely ignore stale completions.
In `@packages/flutter_sdk/pubspec.yaml`:
- Around line 11-15: The pubspec currently uses a local path dependency
"absmartly_dart: path: ../dart_sdk" which blocks publishing; either replace that
entry with a hosted or git dependency (e.g., change absmartly_dart to a git or
hosted version) if you intend to publish, or mark the package private by adding
publish_to: none at the top of pubspec.yaml; update the dependency declaration
or add the publish_to field accordingly and ensure the dependency name
"absmartly_dart" and its declaration are the only changes so tooling picks up
the new source.
- Around line 7-9: pubspec.yaml's environment block has a Flutter constraint
(flutter: ">=3.3.0") that is incompatible with the Dart SDK constraint (sdk:
'>=2.18.6'); update the Flutter constraint to ">=3.3.10" so it matches the Dart
SDK requirement. Open the environment section in pubspec.yaml and change the
flutter version string in the line containing flutter: ">=3.3.0" to flutter:
">=3.3.10", then save and run pub get to verify compatibility.
In `@packages/flutter_sdk/test/platform_test.dart`:
- Around line 227-239: The test currently awaits both futures and compares their
resolved values, which doesn't prove the same Future instance is returned;
instead assert the futures themselves are identical: after obtaining future1 and
future2 from testContext.waitUntilReady(), use expect(future1, same(future2))
(or an equivalent identity check) to verify waitUntilReady returns the same
Future instance; locate this in the test named "context waitUntilReady returns
same future when called multiple times" using symbols testContext and
waitUntilReady.
---
Nitpick comments:
In `@packages/dart_sdk/test/default_http_client_test.dart`:
- Around line 106-111: The test currently verifies mockHttpClient.post/put with
loose matchers (any and anyNamed('body')) allowing wrong endpoints or payloads;
update the assertions to verify the expected URI and exact body content instead
of any (e.g., replace any for the first positional arg with the concrete Uri
used in the code under test and replace anyNamed('body') with a matcher or
string equal to the expected JSON/body), keep headers asserted with
anyNamed('headers') if headers vary or match specific headers if deterministic;
apply the same tightening to the mockHttpClient.put verification (use
mockHttpClient.post and mockHttpClient.put as the identifying symbols to locate
the assertions).
In `@packages/flutter_sdk/README.md`:
- Around line 126-131: The example using context.getTreatment assumes a non-null
Context, but earlier signatures show Context?; update the snippet or add a short
note clarifying null-safety: either show the null-check/unwrapping pattern
(e.g., guard for context == null or use a local non-null variable) before
calling context.getTreatment, or explicitly state "assuming context is non-null"
so readers know to handle Context? in code paths that may yield null; reference
the nullable type Context? and the getTreatment call when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c25610d5-bcfc-495a-9aec-899d83838be5
📒 Files selected for processing (15)
packages/dart_sdk/lib/src/absmartly_sdk_config.dartpackages/dart_sdk/lib/src/context.dartpackages/dart_sdk/lib/src/json/experiment.dartpackages/dart_sdk/lib/src/json/publish_event.dartpackages/dart_sdk/test/default_http_client_test.dartpackages/flutter_sdk/README.mdpackages/flutter_sdk/lib/src/widgets/treatment.dartpackages/flutter_sdk/lib/src/widgets/treatment_builder.dartpackages/flutter_sdk/lib/src/widgets/treatment_switch.dartpackages/flutter_sdk/lib/src/widgets/variable_value.dartpackages/flutter_sdk/pubspec.yamlpackages/flutter_sdk/test/error_recovery_test.dartpackages/flutter_sdk/test/fix_verification_test.dartpackages/flutter_sdk/test/platform_test.dartpackages/flutter_sdk/test/widgets/treatment_test.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/flutter_sdk/test/fix_verification_test.dart
- packages/flutter_sdk/lib/src/widgets/treatment_builder.dart
- packages/dart_sdk/lib/src/absmartly_sdk_config.dart
- packages/flutter_sdk/test/widgets/treatment_test.dart
Comment on lines
+619
to
621
| if (experimentMatches(experiment.data, assignment) && | ||
| audienceMatches(experiment.data, assignment)) { | ||
| // assignment up-to-date |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/dart_sdk/lib/src/context.dart"
echo "== experimentMatches reads late fields =="
nl -ba "$FILE" | sed -n '572,585p'
echo
echo "== cache-retention path that calls experimentMatches =="
nl -ba "$FILE" | sed -n '815,838p'
echo
echo "== Assignment fields currently declared as late =="
nl -ba "$FILE" | sed -n '896,906p'Repository: absmartly/flutter-sdk
Length of output: 166
🏁 Script executed:
#!/bin/bash
FILE="packages/dart_sdk/lib/src/context.dart"
# Check if file exists and get its size
echo "=== File info ==="
wc -l "$FILE"
echo
echo "=== Lines 619-621 (evaluation context) ==="
sed -n '619,621p' "$FILE"
echo
echo "=== Lines 815-838 (cache-retention path) ==="
sed -n '815,838p' "$FILE"
echo
echo "=== Lines 896-906 (Assignment field declarations) ==="
sed -n '896,906p' "$FILE"
echo
echo "=== Search for experimentMatches function definition ==="
rg -n "def experimentMatches|experimentMatches\(" "$FILE" | head -20Repository: absmartly/flutter-sdk
Length of output: 1426
🏁 Script executed:
#!/bin/bash
FILE="packages/dart_sdk/lib/src/context.dart"
echo "=== experimentMatches function definition (line 572+) ==="
sed -n '572,600p' "$FILE"
echo
echo "=== Search for Assignment constructor and placeholder creation ==="
rg -n "Assignment()" "$FILE"
echo
echo "=== Look for where assignments are cached/retrieved ==="
rg -n -B2 -A2 "assignmentCache_\[" "$FILE" | head -40Repository: absmartly/flutter-sdk
Length of output: 2981
🏁 Script executed:
#!/bin/bash
FILE="packages/dart_sdk/lib/src/context.dart"
echo "=== Complete getAssignment function ==="
sed -n '601,711p' "$FILE"
echo
echo "=== Search for where iteration and fullOnVariant are assigned ==="
rg -n "\.iteration\s*=" "$FILE"
echo
echo "=== Search for where fullOnVariant is assigned ==="
rg -n "\.fullOnVariant\s*=" "$FILE"Repository: absmartly/flutter-sdk
Length of output: 4392
🏁 Script executed:
#!/bin/bash
FILE="packages/dart_sdk/lib/src/context.dart"
echo "=== All references to assignment.iteration ==="
rg -n "assignment\.iteration" "$FILE"
echo
echo "=== All references to assignment.fullOnVariant ==="
rg -n "assignment\.fullOnVariant" "$FILE"
echo
echo "=== Check getVariableAssignment function that might access late fields ==="
sed -n '713,730p' "$FILE"Repository: absmartly/flutter-sdk
Length of output: 1111
Guard cached placeholder assignments before calling experimentMatches(...).
experimentMatches(...) at lines 576–577 reads Assignment.iteration and Assignment.fullOnVariant, but these are late fields (lines 898–899). When an experiment is absent, getAssignment(...) creates and caches a placeholder assignment without initialising these fields (lines 628 and 708). Later, if the cache cleanup or evaluation at lines 619 and 833 calls experimentMatches(...) on this cached placeholder, a LateInitializationError is raised.
💡 Proposed fix
class Assignment {
int id = 0;
- late int iteration;
- late int fullOnVariant;
+ int iteration = 0;
+ int fullOnVariant = 0;
late String name;
String? unitType;
List<double> trafficSplit = [];
@@
assignmentCache_.removeWhere((experimentName, assignment) {
if (assignment.overridden) {
return false;
}
final ExperimentVariables? experiment = index[experimentName];
if (experiment == null) {
- return assignment.assigned;
+ return true; // drop stale placeholders for missing experiments
}
+ if (!assignment.assigned) {
+ return true; // avoid matching placeholders with incomplete assignment state
+ }
return !experimentMatches(experiment.data, assignment);
});📝 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
| if (experimentMatches(experiment.data, assignment) && | |
| audienceMatches(experiment.data, assignment)) { | |
| // assignment up-to-date | |
| class Assignment { | |
| int id = 0; | |
| int iteration = 0; | |
| int fullOnVariant = 0; | |
| late String name; | |
| String? unitType; | |
| List<double> trafficSplit = []; | |
| ... | |
| assignmentCache_.removeWhere((experimentName, assignment) { | |
| if (assignment.overridden) { | |
| return false; | |
| } | |
| final ExperimentVariables? experiment = index[experimentName]; | |
| if (experiment == null) { | |
| return true; // drop stale placeholders for missing experiments | |
| } | |
| if (!assignment.assigned) { | |
| return true; // avoid matching placeholders with incomplete assignment state | |
| } | |
| return !experimentMatches(experiment.data, assignment); | |
| }); |
Comment on lines
+302
to
+309
| expect( | ||
| () => clientWithRetries.get( | ||
| 'https://example.com/api/posts', | ||
| null, | ||
| null, | ||
| ), | ||
| throwsException, | ||
| ); |
There was a problem hiding this comment.
Assert retry attempt count in the max-retry exception test.
Line 302 only checks that an exception is thrown; this test would still pass if retries silently dropped to a single attempt.
Small assertion upgrade
expect(
() => clientWithRetries.get(
'https://example.com/api/posts',
null,
null,
),
throwsException,
);
+ verify(mockHttpClient.get(any, headers: anyNamed('headers'))).called(2);📝 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
| expect( | |
| () => clientWithRetries.get( | |
| 'https://example.com/api/posts', | |
| null, | |
| null, | |
| ), | |
| throwsException, | |
| ); | |
| expect( | |
| () => clientWithRetries.get( | |
| 'https://example.com/api/posts', | |
| null, | |
| null, | |
| ), | |
| throwsException, | |
| ); | |
| verify(mockHttpClient.get(any, headers: anyNamed('headers'))).called(2); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dart_sdk/test/default_http_client_test.dart` around lines 302 - 309,
The test currently only asserts that clientWithRetries.get throws, but must also
assert that the client retried the expected number of times; update the test to
capture the thrown exception (e.g., using try/catch or expectLater that returns
the error) and then assert the retry count on the mock underlying client or
retry counter equals the expected attempts (e.g., verify/mockCallCount for the
underlying HTTP call was invoked maxRetries + 1 times) referencing
clientWithRetries.get and the mock underlying client or retry policy variable
used in this test.
Comment on lines
+53
to
+77
| ### Synchronously | ||
|
|
||
| ```dart | ||
| final ContextConfig contextConfig = ContextConfig.create() | ||
| .setUnit("session_id", "5ebf06d8cb5d8137290c4abb64155584fbdb64d8"); | ||
|
|
||
| final Context? context = await sdk.createContext(contextConfig).waitUntilReady(); | ||
|
|
||
| if (context != null) { | ||
| print("context ready"); | ||
| } | ||
| ``` | ||
|
|
||
| ### Asynchronously | ||
|
|
||
| ```dart | ||
| final ContextConfig contextConfig = ContextConfig.create() | ||
| .setUnit("session_id", "5ebf06d8cb5d8137290c4abb64155584fbdb64d8"); | ||
|
|
||
| final Context? context = await sdk.createContext(contextConfig).waitUntilReady(); | ||
|
|
||
| if (context != null) { | ||
| print("context ready"); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
"Synchronously" and "Asynchronously" sections are identical.
The code examples in both sections are byte-for-byte identical, both using await and async patterns. This is confusing for developers trying to understand the difference between synchronous and asynchronous context creation. Either differentiate these sections with distinct code patterns or remove the duplicate section.
📝 Clarification needed
If "Synchronously" means using waitUntilReady() to block until ready (but still async), and "Asynchronously" means creating the context without waiting, the examples should be:
Synchronously (wait for ready):
final Context? context = await sdk.createContext(contextConfig).waitUntilReady();Asynchronously (don't wait):
final Context context = sdk.createContext(contextConfig);
// context may not be ready yet, check with context.isReady()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/README.md` around lines 53 - 77, The two examples under
"Synchronously" and "Asynchronously" are identical; update them to show the real
difference: in the synchronous example use
sdk.createContext(contextConfig).waitUntilReady() (references: ContextConfig,
sdk.createContext, waitUntilReady, Context) and in the asynchronous example show
non-blocking creation (references: ContextConfig, sdk.createContext, Context)
and explain that the returned Context may not be ready yet and should be checked
with context.isReady() instead of awaiting; remove the duplicate code and ensure
each section demonstrates the correct pattern and mentions isReady() for the
async case.
Comment on lines
+101
to
+127
| testWidgets( | ||
| 'timeout fallback shows control after readyTimeout in placeholder mode', | ||
| (tester) async { | ||
| final contextConfig = ContextConfig.create() | ||
| ..setUnit('user_id', '12345'); | ||
| final context = sdk.createContext(contextConfig); | ||
|
|
||
| await tester.pumpWidget( | ||
| ABSmartlyProvider( | ||
| sdk: sdk, | ||
| context: context, | ||
| defaultLoadingBehavior: LoadingBehavior.placeholder, | ||
| readyTimeout: const Duration(milliseconds: 50), | ||
| child: Treatment( | ||
| name: 'test_experiment', | ||
| variants: { | ||
| 0: const Text('Control After Timeout', | ||
| textDirection: TextDirection.ltr), | ||
| 1: const Text('Variant', textDirection: TextDirection.ltr), | ||
| }, | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| await tester.pump(); | ||
| await tester.pump(const Duration(milliseconds: 100)); | ||
| }); |
There was a problem hiding this comment.
Add a real assertion to the timeout fallback test.
This test currently has no expect, so it can’t fail even if timeout fallback breaks.
Suggested fix
await tester.pump();
await tester.pump(const Duration(milliseconds: 100));
+ expect(find.text('Control After Timeout'), findsOneWidget);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/test/error_recovery_test.dart` around lines 101 - 127,
The test "timeout fallback shows control after readyTimeout in placeholder mode"
lacks any assertion; add an expect to verify the fallback control is displayed
after the readyTimeout. After the second pump (await tester.pump(const
Duration(milliseconds: 100));) use tester's finder to locate the Text widget
with "Control After Timeout" (or verify that a widget matching the control
variant for Treatment 'test_experiment' is present) and assert it is found
(e.g., expect(found, findsOneWidget)); keep the check tied to the
ABSmartlyProvider/Treatment setup and readyTimeout/defaultLoadingBehavior used
in the test.
Comment on lines
+273
to
+295
| testWidgets('isFailed reflects context state', (tester) async { | ||
| final contextConfig = ContextConfig.create() | ||
| ..setUnit('user_id', '12345'); | ||
| final context = sdk.createContext(contextConfig); | ||
|
|
||
| bool? capturedIsFailed; | ||
|
|
||
| await tester.pumpWidget( | ||
| ABSmartlyProvider( | ||
| sdk: sdk, | ||
| context: context, | ||
| child: Builder( | ||
| builder: (ctx) { | ||
| final data = ABSmartlyProvider.of(ctx); | ||
| capturedIsFailed = data.isFailed; | ||
| return const SizedBox(); | ||
| }, | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| expect(capturedIsFailed, isNotNull); | ||
| }); |
There was a problem hiding this comment.
The assertion does not validate the test name’s claim.
The test says isFailed reflects context state, but only checks non-null. Compare against the context state directly.
Suggested fix
- expect(capturedIsFailed, isNotNull);
+ expect(capturedIsFailed, equals(context.isFailed()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/test/error_recovery_test.dart` around lines 273 - 295,
The test `isFailed reflects context state` currently only asserts
capturedIsFailed is not null; change it to assert the provider value matches the
context's failure state by comparing capturedIsFailed to the created context's
actual state (e.g. replace expect(capturedIsFailed, isNotNull) with
expect(capturedIsFailed, equals(context.isFailed)) or the appropriate context
property), locating the values via
ContextConfig.create()/setUnit('user_id','12345'), sdk.createContext(...) and
ABSmartlyProvider.of(...) / capturedIsFailed.
Comment on lines
+332
to
+354
| testWidgets('context waitUntilReady can be called multiple times', | ||
| (tester) async { | ||
| final contextConfig = ContextConfig.create() | ||
| ..setUnit('user_id', '12345'); | ||
| final context = sdk.createContext(contextConfig); | ||
|
|
||
| await tester.pumpWidget( | ||
| ABSmartlyProvider( | ||
| sdk: sdk, | ||
| context: context, | ||
| child: Builder( | ||
| builder: (ctx) { | ||
| final data = ABSmartlyProvider.of(ctx); | ||
| data.context.waitUntilReady(); | ||
| data.context.waitUntilReady(); | ||
| return const SizedBox(); | ||
| }, | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| await tester.pump(); | ||
| }); |
There was a problem hiding this comment.
waitUntilReady retry test is effectively a no-op.
It doesn’t verify idempotency/safety; it only calls the method. Capture both futures and assert identity.
Suggested fix
testWidgets('context waitUntilReady can be called multiple times',
(tester) async {
final contextConfig = ContextConfig.create()
..setUnit('user_id', '12345');
final context = sdk.createContext(contextConfig);
+ Future<void>? f1;
+ Future<void>? f2;
await tester.pumpWidget(
ABSmartlyProvider(
sdk: sdk,
context: context,
child: Builder(
builder: (ctx) {
final data = ABSmartlyProvider.of(ctx);
- data.context.waitUntilReady();
- data.context.waitUntilReady();
+ f1 = data.context.waitUntilReady();
+ f2 = data.context.waitUntilReady();
return const SizedBox();
},
),
),
);
await tester.pump();
+ expect(f1, isNotNull);
+ expect(f1, same(f2));
});📝 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
| testWidgets('context waitUntilReady can be called multiple times', | |
| (tester) async { | |
| final contextConfig = ContextConfig.create() | |
| ..setUnit('user_id', '12345'); | |
| final context = sdk.createContext(contextConfig); | |
| await tester.pumpWidget( | |
| ABSmartlyProvider( | |
| sdk: sdk, | |
| context: context, | |
| child: Builder( | |
| builder: (ctx) { | |
| final data = ABSmartlyProvider.of(ctx); | |
| data.context.waitUntilReady(); | |
| data.context.waitUntilReady(); | |
| return const SizedBox(); | |
| }, | |
| ), | |
| ), | |
| ); | |
| await tester.pump(); | |
| }); | |
| testWidgets('context waitUntilReady can be called multiple times', | |
| (tester) async { | |
| final contextConfig = ContextConfig.create() | |
| ..setUnit('user_id', '12345'); | |
| final context = sdk.createContext(contextConfig); | |
| Future<void>? f1; | |
| Future<void>? f2; | |
| await tester.pumpWidget( | |
| ABSmartlyProvider( | |
| sdk: sdk, | |
| context: context, | |
| child: Builder( | |
| builder: (ctx) { | |
| final data = ABSmartlyProvider.of(ctx); | |
| f1 = data.context.waitUntilReady(); | |
| f2 = data.context.waitUntilReady(); | |
| return const SizedBox(); | |
| }, | |
| ), | |
| ), | |
| ); | |
| await tester.pump(); | |
| expect(f1, isNotNull); | |
| expect(f1, same(f2)); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/flutter_sdk/test/error_recovery_test.dart` around lines 332 - 354,
The test 'context waitUntilReady can be called multiple times' currently just
calls data.context.waitUntilReady() twice; change it to capture both returned
Futures (e.g., final f1 = data.context.waitUntilReady(); final f2 =
data.context.waitUntilReady();) and then assert they are identical (expect(f1,
same(f2))) and/or await both to ensure the same readiness result; update the
test body where data.context.waitUntilReady() is invoked to use these captured
futures and assertions to verify idempotency/safety of waitUntilReady.
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
Test plan
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
Chore