Conversation
There was a problem hiding this comment.
Pull request overview
Adds planning documentation for FFL-2837 (“dynamic/rules-based offline init”) in the React Native SDK, outlining intended behavior, dependency needs in @datadog/flagging-core, risks, and a proposed RN implementation + test plan.
Changes:
- Add a detailed technical plan covering architecture, upstream gaps, risk analysis, and step-by-step implementation/testing.
- Add a Simplified Technical English version of the plan for broader review/accessibility.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dynamic_offline.plan.md | Detailed implementation plan, upstream dependency analysis, and risks/test plan for rules-based offline evaluation. |
| dynamic_offline_simplified.plan.md | Simplified Technical English version of the same plan for clearer cross-team review. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Use this existing flow: | ||
|
|
||
| ```text | ||
| configurationFromString -> setConfiguration -> evaluate |
| @@ -0,0 +1,772 @@ | |||
| # FFL-2837 — Dynamic (rules-based) offline init in dd-sdk-reactnative | |||
|
|
|||
| **Jira:** [FFL-2837 — Building Blocks API for dynamic offline init in ReactNative SDK](https://datadoghq.atlassian.net/browse/FFL-2837) | |||
| - [Portable Flag Configuration RFC](https://docs.google.com/document/d/1OWNBtXtSk535VXqf-9fqsAmU9W8kpFLAwxYi2y1qyQQ/edit?pli=1&tab=t.0#heading=h.n52036mkzewg) (local snapshot: `./Portable-Flag-Configuration-RFC.md`, repo root, untracked) — defines the building blocks: `ConfigurationWire`, `configurationFromString/ToString`, `CoreProvider`, fetch fns, hooks. | ||
| - [Offline Initialization for Feature Flagging RFC](https://docs.google.com/document/d/1q1GlEbAgCGuO1OWfGbmKQkk5Oo-rE7YQwq29kMJJ4II/edit?pli=1&tab=t.0#heading=h.rnd972k0hiyer) (local snapshot: `./Offline-Initialization-for-Feature-Flagging.md`, repo root, untracked) — offline recipes built from those blocks; the operation is always `configurationFromString(wire) → provider.setConfiguration(config) → evaluate(...)`. | ||
| - [ConfigurationWire (Confluence)](https://datadoghq.atlassian.net/wiki/spaces/PANA/pages/5141725646/ConfigurationWire) — the published wire spec. Its **protobuf/base64** rules encoding is the intended target (see §2.5), even though the current code still uses JSON. | ||
| - **RFC: Obfuscation for rules-based client configs** (local: `./RFC_Obfuscation_for_rules-based_client configs.md`, 2026-07-10, first draft, one approval) — defines the obfuscation design for client rules: per-flag opt-in switch, salted `ONE_OF_SHA256`/`NOT_ONE_OF_SHA256` operators (server-compatible, engine-evaluated), binary structure format, and "document what's exposed". Answers most of our obfuscation open question — see G6/D7. |
| # FFL-2837 — Dynamic Offline PR Stack | ||
|
|
||
| This document uses Simplified Technical English. |
|
|
||
| FFL-2837 adds the **rules-based** (dynamic) offline flow: | ||
|
|
||
| - Customer loads a **rules-based** configuration (Universal Flag Configuration) via `setConfiguration` |
There was a problem hiding this comment.
So setConfiguration can be used both to set a precomputed flag config and also the rules used to evaluate and compute it against a context?
Is there any particular reason for this and not have a specific and more clearly named function to set the rules?
I fear the API might end up being too broad for customers to clearly understand what does what and which precise calls they need to make to achieve the setup they want.
|
|
||
| --- | ||
|
|
||
| ## 2. Current state of `@datadog/flagging-core` |
There was a problem hiding this comment.
What's the expected size footprint of this package?, also, is this module stable?, we need to keep in mind that we don't do major releases that often so if any functionality of this module is to be exposed on the public SDK API we need to be sure that this does not introduce breaking changes often.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
dynamic_offline_pr_stack.plan.md:3
- The PR description says this PR adds
dynamic_offline.plan.md(detailed plan), but the branch currently addsdynamic_offline_pr_stack.plan.mdinstead. This mismatch makes it hard for reviewers to find the detailed plan document; either add/rename the detailed plan file as described, or update the PR description to match the actual files in the PR.
# FFL-2837 — Dynamic Offline PR Stack
This document uses Simplified Technical English.
4878144 to
7b00adc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
dynamic_offline_simplified.plan.md:19
- This “Upstream PR #344” reference is ambiguous without an explicit repo/link (it can be read as a PR in this repo). Consider fully qualifying it like other cross-repo references.
Upstream PR #344 now defines the expected implementation contract.
dynamic_offline_simplified.plan.md:9
- The PR references are written as plain “#NNN”, which is ambiguous inside this repository (e.g., “#344” could be interpreted as a dd-sdk-reactnative PR). Using explicit links here will keep the plan unambiguous over time.
This issue also appears on line 19 of the same file.
**Upstream references:** DataDog/openfeature-js-client PRs #343, #344, and #336
dynamic_offline_pr_stack.plan.md:26
- These references to “Upstream PR #344” / “PR #336” are ambiguous without a repo name/link (they can be interpreted as PRs in this repo). Using explicit links will make the stack plan clearer for readers.
Upstream PR #344 adds the generated Protobuf-ES rules parser, SHA-256 evaluation, validation, safe flag lookup, and React Native compatibility.
It adds `@bufbuild/protobuf` as a runtime dependency.
Its packed-package smoke test uses the Metro export conditions from this repository.
Upstream PR #336 uses that parser in the browser `CoreProvider`.
PR #336 also uses the safe upstream lookup for precomputed flags.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
dynamic_offline_simplified.plan.md:21
- “Upstream PR #344” is ambiguous; qualify it with the upstream repository name to avoid confusion with PR numbers in this repo.
The RFC documents are drafts.
Upstream PR #344 now defines the expected implementation contract.
Names and versions can still change before publication.
dynamic_offline_pr_stack.plan.md:35
- Similarly, qualify “PR #336” with the upstream repository name to make it clear this is referring to DataDog/openfeature-js-client, not this repo.
Upstream PR #336 uses that parser in the browser `CoreProvider`.
PR #336 also uses the safe upstream lookup for precomputed flags.
dynamic_offline_simplified.plan.md:9
- References like “PR #344” are ambiguous in this repo (there is also a PR #344 here). Use an explicit repo-qualified reference so readers know this is DataDog/openfeature-js-client.
This issue also appears on line 19 of the same file.
**Upstream references:** DataDog/openfeature-js-client PRs #343, #344, and #336; ddoghq/dd-source PR #34959
dynamic_offline_pr_stack.plan.md:26
- Upstream PR numbers are ambiguous in this repo. Qualify “PR #344” with the upstream repository name (DataDog/openfeature-js-client) so it can’t be confused with dd-sdk-reactnative PR numbers.
This issue also appears on line 34 of the same file.
Published flagging-core version 2.0.2 does not contain the new rules wire contract.
Upstream PR #344 adds the generated Protobuf-ES rules parser, SHA-256 evaluation, validation, safe flag lookup, and React Native compatibility.
It adds `@bufbuild/protobuf` as a runtime dependency.
Its packed-package smoke test uses the Metro export conditions from this repository.
It moves wire parsing and `FlagsConfigurationWire` to `@datadog/flagging-core/configuration`.
The default flagging-core entry point keeps the evaluator and shared types.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
dynamic_offline_simplified.plan.md:21
- The references like “Upstream PR #344” are ambiguous in this repo because PR numbers also exist locally (e.g., this repository has its own PR #344). Qualify upstream PR references with the repository name (or a full link) at the point of use to avoid confusion for readers.
The RFC documents are drafts.
Upstream PR #344 now defines the expected implementation contract.
Names and versions can still change before publication.
dynamic_offline_pr_stack.plan.md:22
- This document refers to “Upstream PR #344” / “PR #336” by number only. Since this repository also has its own PR numbers, please qualify these references with the upstream repo (e.g., DataDog/openfeature-js-client#344) so the stack plan is unambiguous.
Published flagging-core version 2.0.2 does not contain the new rules wire contract.
Upstream PR #344 adds the generated Protobuf-ES rules parser, SHA-256 evaluation, evaluation-time validation, safe flag lookup, and React Native compatibility.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings in the implementation plans must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (7)
dynamic_offline_pr_stack.plan.md:232
- This conflicts with the stated user-visible change:
{}andclearContext()must no longer restore the embedded precomputed context, because callers explicitly usegetPrecomputedContext(configuration)when they want that context. Narrow this bullet to precomputed flag evaluation/cache behavior and explicitly exempt the provider context lifecycle.
- Keep online and precomputed behavior unchanged.
dynamic_offline_pr_stack.plan.md:258
- The current provider tests explicitly assert that
{}andclearContext()re-adopt the embedded precomputed context (packages/react-native-openfeature/src/__tests__/offlineProvider.test.ts:60-74, 143-169), but this feature changes that contract. These listed tests do not require updating those assertions or covering the explicitgetPrecomputedContextpath, so a regression can pass; add both cases to the PR3 test plan.
- Add global-context and domain-context tests.
- Confirm the Web SDK 1.8 hook-context constraint.
dynamic_offline_pr_stack.plan.md:218
- These bullets introduce a configuration-level
PARSE_ERROR, but the current coreConfigurationErrorCodeonly hasGENERALfor unusable configurations and the React Native provider'sOF_ERROR_CODE/toErrormapping has noPARSE_ERRORbranch (packages/core/src/flags/FlagsClient.ts:29-32,packages/react-native-openfeature/src/offlineProvider.ts:40-44,169-181). Add the explicit core/provider error-contract changes to the relevant PR scope; otherwise branch failures will still surface asGENERAL.
- Return `PROVIDER_NOT_READY` only when no configuration was supplied.
- Return `PARSE_ERROR` when a supplied configuration has no usable capability.
dynamic_offline_simplified.plan.md:684
- This is a public API decision, but the PR stack does not assign it to a PR or list it as remaining work. Upgrading the already-public
ParsedFlagsConfigurationalias will exposerules.response, while choosing a branded type is a breaking change; PR1 should resolve and test this before publishing the new configuration surface.
Select one API policy:
1. Accept structural visibility and remove claims of opacity.
2. Make `ParsedFlagsConfiguration` a branded type.
dynamic_offline_simplified.plan.md:1305
- G12 is explicitly incomplete: salt length, configuration/condition/value limits, and cross-SDK vectors are still unspecified, while D7 says complete support must not be claimed until G12 is complete. Neither the PR description's remaining-work list nor PR1–PR3 makes this a gate, so the stack can ship SHA operators with an unresolved protocol/resource contract. Add G12 as a blocking prerequisite or narrow the stack's scope and claims.
Do not claim complete support until G12 is complete.
Let the upstream evaluator report unsupported flag data.
Return the upstream `PARSE_ERROR` for an unsupported flag.
Use canonical test vectors.
dynamic_offline_simplified.plan.md:730
- This requirement cannot be met by the current error contract as written:
ConfigurationErrorCodeonly includesGENERALfor an unusable loaded configuration, and the OpenFeature provider maps that case toGeneralError. The plan should explicitly assign the new configuration-levelPARSE_ERRORcode and update both core and provider mappings, otherwise the mixed-branch tests below will fail.
Return `PROVIDER_NOT_READY` only when no configuration was supplied.
Return `PARSE_ERROR` when a supplied configuration has no usable branch.
dynamic_offline_simplified.plan.md:692
- The customer-facing behavior relies on
getPrecomputedContext(configuration)so precomputed users can explicitly set their context, but this step only requires re-exporting the parser functions. Following it would leave the helper (and the opaqueFlagsConfigurationWiretype) unavailable from the React Native API even though the plan documents both as supported exports. Require those public re-exports and contract tests here.
Import and re-export the upstream conversion functions from `@datadog/flagging-core/rules-based`.
- Files reviewed: 2/2 changed files
- Comments generated: 9
- Review effort level: Lite
| - Keep OpenFeature types out of React Native core. | ||
| - Use compatible internal context and logger types. |
| - Keep OpenFeature types out of React Native core. | ||
| - Use compatible internal context and logger types. | ||
| - Verify that the pinned evaluator returns `FLAG_NOT_FOUND` for absent reserved-name keys. | ||
| - Keep regular-expression safety as an explicit open item. |
| Do not claim that structural validation stops ReDoS. | ||
| PR #344 compiles regular expressions lazily and caches the result by configuration and regex index. | ||
| This reduces repeated compilation. | ||
| It does not make hostile expressions safe. | ||
| Select one regular-expression protection: | ||
|
|
||
| - An upstream safe-regex guarantee | ||
| - A static safe-regex policy | ||
| - A bounded regular-expression engine |
| The flagging-core declaration files import `@openfeature/core`. | ||
| Flagging-core lists that package as a development dependency, not as a runtime or peer dependency. | ||
| Local hoisting hides this problem. | ||
|
|
||
| Select one solution: | ||
|
|
||
| 1. Keep OpenFeature types in `react-native-openfeature`. | ||
| 2. Pass compatible internal context and logger types to core. | ||
| 3. Or, add an explicit core dependency. | ||
| 4. If you add the dependency, fix the flagging-core package dependency too. |
| React Native: { targetingKey, attributes } | ||
| OpenFeature: { targetingKey, ...attributes } | ||
| ``` |
| 3. Otherwise, if precomputed data exists, return `INVALID_CONTEXT`. | ||
| 4. Otherwise, return `PROVIDER_NOT_READY`. |
| **Recommended decision:** Reserve `id` for `targetingKey`. | ||
|
|
||
| Drop a customer `id` attribute in the flat adapter. | ||
| Make rule matching and sharding use one subject identifier. |
| Rewrite the offline README section. | ||
| Show the precomputed flow and the rules flow separately. | ||
| Apply each warning only to the applicable flow. |
|
|
||
| ### Q1 — Published flagging-core version | ||
|
|
||
| - [ ] Identify the version that contains rules wire parsing. |
Planning doc for rules-based (dynamic) offline feature-flag init in the React Native SDK: gaps to bridge in @datadog/flagging-core, explicit implementation steps, test plan, and risks/unknowns. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve five of the plan's open questions: gate rules exposure on doLog (D3), keep two evaluation paths (D4), accept static-import bundle cost and reject dynamic import (D5), no offline opt-in gate for rules (D6), treat obfuscation/hashing as upstream (D7). Punt the flagging-core version/protobuf questions (Q1-Q2) to coordinate with upstream owners. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Point the Portable Flag Configuration and Offline Initialization references at their source Google Docs, keeping the local snapshot filenames noted alongside. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fold in the Obfuscation RFC (salted ONE_OF_SHA256 operators + binary structure) and the round-5/6 review findings. Obfuscation operators are absent from 2.0.1 and today silently fall back to DEFAULT, so unknown operators must be rejected as GENERAL, derived from the pinned OperatorType rather than an RN-maintained set. Sync SHA-256 is new bundle mass (Hermes+JSC). The hash protocol is unspecified and needs cross-SDK vectors plus malformed-condition load validation. Unsupported operators invalidate the rules branch only, keeping a valid precomputed sibling. Corrected the threat model and the full "what stays visible" UFC list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d27c9a7 to
6f481ac
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved configuration-error handling and safety, identity, packaging, API-surface, and context-contract issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (9)
dynamic_offline_pr_stack.plan.md:150
- Keeping regex safety as an open item while PR3 is explicitly tasked with exposing customer-supplied rules leaves a synchronous JavaScript-thread denial-of-service path without a release gate. Move the selected bounded policy (or rejection of regex rules) into PR1/PR3 acceptance criteria and require the hostile-expression test before activation.
- Verify that the pinned evaluator returns `FLAG_NOT_FOUND` for absent reserved-name keys.
- Keep regular-expression safety as an explicit open item.
- Add adapter contract tests.
dynamic_offline_pr_stack.plan.md:215
- The stack does not carry the D9 subject-identity decision into PR2: the rules evaluator can match on
idwhile sharding ontargetingKey, which changes assignments. Add an explicit PR2 prerequisite and contract test requiring one defined subject policy before implementing path selection.
- Reconcile a rules branch as ready for each context.
- Select the evaluation path for each resolution.
- Use matching precomputed data first.
- Use valid rules data second.
dynamic_offline_simplified.plan.md:550
- Because
rules.responseis customer-supplied and regex evaluation runs synchronously on the JavaScript thread, leaving ReDoS protection as an open follow-up allows the PR3 activation path to expose a configuration that can block the app. Make a bounded policy (or explicit rejection of regex rules) a prerequisite for activation and gate it with the hostile-expression test before treating the stack as complete.
Do not claim that structural validation stops ReDoS.
PR #344 compiles regular expressions lazily and caches the result by configuration and regex index.
This reduces repeated compilation.
It does not make hostile expressions safe.
dynamic_offline_simplified.plan.md:788
- Rule matching and sharding can use different subjects when
iddiffers fromtargetingKey, so this choice changes flag assignments rather than only the adapter shape. D9 is still only a recommendation while PR2 already schedules evaluation and path-selection work; resolve and test this policy before PR2 instead of allowing an implementation-dependent result.
Decide the `id` policy.
The current evaluator lets a custom `id` replace the targeting-key `id`.
Sharding still uses `targetingKey`.
This can use two subject identifiers.
dynamic_offline_simplified.plan.md:599
- The declaration files' import of
@openfeature/coreis a concrete packed-install failure mode, not only a design choice: flagging-core declares it as a development dependency, so a consumer that does not hoist workspace dependencies may fail to resolve the public types. Move this decision into the PR1 prerequisites and add a packed-install/typecheck gate before exposing the new configuration type.
The flagging-core declaration files import `@openfeature/core`.
Flagging-core lists that package as a development dependency, not as a runtime or peer dependency.
Local hoisting hides this problem.
dynamic_offline_simplified.plan.md:294
- This order skips the supplied-but-invalid configuration case: when parsing leaves only
configurationError/rulesError/precomputedError(or no usable branch), the flow falls through toPROVIDER_NOT_READY, contradicting lines 729–731 and the stated contract that a supplied unusable configuration returnsPARSE_ERROR. Distinguish a valid precomputed mismatch from a supplied configuration with no usable capability before the not-ready case.
1. If precomputed data matches the effective context, use its decoded `Map` or its per-flag parse error.
2. Otherwise, if rules data exists, evaluate the rules.
3. Otherwise, if precomputed data exists, return `INVALID_CONTEXT`.
4. Otherwise, return `PROVIDER_NOT_READY`.
dynamic_offline_simplified.plan.md:693
- Step 0 leaves whether the React Native root continues to export configuration parsing as an open bundle/API decision, but this step unconditionally re-exports the
/rules-basedconversion functions. Because the current public root exports these functions from this module (packages/core/src/index.tsx:35-39, 74-82), implementing this literally pulls Protobuf-ES into the root and defeats the opt-in boundary being measured in G5. Choose the root-versus-subpath surface before PR1 and place the exports behind that choice.
Do not add React Native parsing code.
Import and re-export the upstream conversion functions from `@datadog/flagging-core/rules-based`.
Do not import the package-root conversion functions for dynamic rules because they intentionally ignore rules.
dynamic_offline_simplified.plan.md:1363
- This prerequisite is already resolved elsewhere in the plan: G1 and Step 0 state that flagging-core 3.0.0 is published and pinned. Leaving Q1 unchecked contradicts the claimed final upstream state and makes the plan appear blocked; mark it complete or revise the surrounding status.
- [ ] Identify the version that contains rules wire parsing.
dynamic_offline_simplified.plan.md:861
- This is ambiguous and conflicts with the current provider behavior in
packages/react-native-openfeature/src/offlineProvider.ts:147-155: an empty context currently callsresetEvaluationContextWithoutFetching(), which re-adopts a precomputed configuration's embedded context. The new contract requires{}/clearContext()to be passed literally, so make that behavior explicit rather than allowing PR3 to preserve the old reset path.
Preserve an empty provider context.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| 4. Put the base64 string in a version `1` `rules.response` envelope. | ||
|
|
||
| The React Native SDK does not call this endpoint. | ||
| It does not add this envelope. |
Pull request stack
This description uses Simplified Technical English. Technical names and API names do not change.
Summary
This PR adds the implementation plans for FFL-2837. FFL-2837 adds dynamic rules-based offline feature flags to the React Native SDK. This PR does not change product code.
dynamic_offline_simplified.plan.mddefines the complete feature plan.dynamic_offline_pr_stack.plan.mddivides the implementation into three reviewable PRs.Feature behavior
setConfiguration.setContextchanges the active context.{}) is a real OpenFeature context.getPrecomputedContext(configuration)and explicitly set that context.Upstream state
@datadog/flagging-core@3.0.0is published.getPrecomputedContext.Implementation stack
FlagsClient.Remaining work
getPrecomputedContexthas a version.@openfeature/core, but flagging-core 3.0.0 lists it only as a development dependency.FlagsConfigurationWire.