Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Cross-package asynchronous lifecycle, weak-reference, and cancellation semantics warrant final human validation despite extensive coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Adds retained first-install flag callbacks across Dart core and Flutter, including cancellation, documentation, examples, and comprehensive tests.
Changes:
- Adds immutable flag events and an event facade backed by weak client associations.
- Implements one-shot asynchronous delivery and cancellation in core and Flutter.
- Updates examples, documentation, and lifecycle/GC test coverage.
| File | Description |
|---|---|
packages/datadog_flags/lib/datadog_flags.dart |
Exports event APIs. |
packages/datadog_flags/lib/datadog_flags_internal.dart |
Exports the companion-package bridge. |
packages/datadog_flags/lib/src/default_flags_client.dart |
Registers core event delivery. |
packages/datadog_flags/lib/src/flags_client.dart |
Clarifies initialization documentation. |
packages/datadog_flags/lib/src/flags_client_event.dart |
Defines immutable event data. |
packages/datadog_flags/lib/src/flags_event_registry.dart |
Adds weak event-source associations. |
packages/datadog_flags/lib/src/flags_events.dart |
Adds the public facade and extension. |
packages/datadog_flags/lib/src/flags_repository.dart |
Retains and delivers the first event. |
packages/datadog_flags/lib/src/no_op_flags_client.dart |
Supports no-op registration. |
packages/datadog_flags/test/flags_events_compatibility_test.dart |
Tests legacy-client compatibility. |
packages/datadog_flags/test/flags_client_event_test.dart |
Tests event immutability and type mapping. |
packages/datadog_flags/test/first_flags_registration_test.dart |
Tests registration and cancellation. |
packages/datadog_flags/test/first_flags_callback_test.dart |
Tests installation callback behavior. |
packages/datadog_flags/test_vm/helpers/force_gc.dart |
Adds VM garbage-collection support. |
packages/datadog_flags/test_vm/first_flags_capture_test.dart |
Tests capture release and facade lifetime. |
packages/datadog_flags/example/bin/typed_evaluation.dart |
Demonstrates first-install callbacks. |
packages/datadog_flags/example/test/typed_evaluation_test.dart |
Tests the CLI example. |
packages/datadog_flags/example/pubspec.yaml |
Adds example test dependencies. |
packages/datadog_flags/README.md |
Documents the core API. |
packages/datadog_flags/CHANGELOG.md |
Records the core feature. |
packages/datadog_flags_flutter/lib/datadog_flags_flutter.dart |
Re-exports event APIs. |
packages/datadog_flags_flutter/lib/src/datadog_flags_plugin.dart |
Forwards registrations to core. |
packages/datadog_flags_flutter/test/helpers/first_flags_test_client.dart |
Adds a wrapper test factory. |
packages/datadog_flags_flutter/test/flags_events_compatibility_test.dart |
Tests Flutter compatibility. |
packages/datadog_flags_flutter/test/first_flags_registration_test.dart |
Tests forwarding and cancellation. |
packages/datadog_flags_flutter/test_vm/helpers/force_gc.dart |
Adds Flutter VM GC support. |
packages/datadog_flags_flutter/test_vm/first_flags_capture_test.dart |
Tests wrapper capture release. |
packages/datadog_flags_flutter/example/lib/main.dart |
Demonstrates callback lifecycle handling. |
packages/datadog_flags_flutter/example/test/first_flags_example_test.dart |
Tests the Flutter example. |
packages/datadog_flags_flutter/example/pubspec.yaml |
Adds example test dependencies. |
packages/datadog_flags_flutter/README.md |
Documents Flutter behavior and release coordination. |
packages/datadog_flags_flutter/CHANGELOG.md |
Records the Flutter feature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sameerank
left a comment
There was a problem hiding this comment.
Codex found one correctness issue but I assume it's non-blocking. Doesn't sound common to register and initialize in different Dart zones
The base branch was changed.
leoromanovsky
left a comment
There was a problem hiding this comment.
Didn't find any actionable defects.
fuzzybinary
left a comment
There was a problem hiding this comment.
The two very big things here that I'm concerned about:
- Calling the GC directly, even in tests. Unless I'm mistaken, that is not publicly documented, and we have no control over what Dart does with that call, or even what type of GC it might trigger.
- The additions both README are way too detailed for public facing documentation and in some cases are specifically instructions to us as maintainers. If we want to retain that information, it needs to go somewhere else.
Notify applications when the first usable flag configuration is installed. Core retains the immutable key inventory and delivers callbacks asynchronously in their registration zones; Flutter bridges registration and cancellation to core. Validate with 148 core and Flutter tests, static analysis, formatting, and compiled CLI and Flutter examples. BREAKING CHANGE: Custom DatadogFlagsClient implementations must implement onFirstFlags.
7a70427 to
852b843
Compare
Updated documentation for onFirstFlags method to clarify behavior.
Updated documentation for FlagsClientEvent constructor.
Use FlagsClientEventListener across core and Flutter callback signatures while preserving inline closure compatibility and unregister behaviour. Validated with 125 core tests, 23 Flutter tests, static analysis, formatting, and public-import checks for both packages.
Retain the source-list mutation check and remove redundant container and collection assertions. First-flags behavioural coverage remains unchanged. Validation: 123 core tests pass; static analysis and formatting pass.
|
Thanks for the detailed review, Jeff. PTAL |
What and why?
Applications need to react when the first usable flag configuration is installed, including cached flags available before network initialization finishes. Add
onFirstFlagsdirectly to the Dart and Flutter flags clients:Each registration receives the first accepted configuration's immutable key inventory once, including an empty list for an empty configuration. Late registrations receive the retained event. If no usable configuration is installed, the registration can remain pending until cancelled. Evaluations in the callback read current flag values.
This is a source-breaking interface addition: custom
DatadogFlagsClientimplementations must implement the method. The core and Flutter package release versions and minimum dependency must be coordinated before publication.How?
Core retains the first event, binds listeners to their registration zones and schedules delivery in a later microtask. The returned unregister function is idempotent and suppresses delivery until the callback starts. Callback references are cleared on cancellation or delivery. Synchronous listener errors are isolated; the application owns errors from asynchronous work started by its callback.
Flutter resolves its underlying client and forwards registration, delivery and cancellation. Cancellation also works while that resolution is pending.
FlagsClientEventkeeps its constructor annotated@internaland defensively copies its key list.The CLI example prints the installed keys and evaluates its selected flag. The Flutter example registers in
initState, displays a callback evaluation and unregisters indispose; it uses the repository's.envconvention.The Flutter app registers and cleans up as follows (exact excerpt):
Local validation: 123 core tests and 23 Flutter tests passed, including controlled microtask/resolver tests and real-core Flutter RUM integration. Both packages and examples analyse cleanly; 49 Dart files pass formatting checks. The CLI executable and Flutter debug bundle build successfully. These checks use companion source packages and do not establish native-device or published-package compatibility.
Review checklist