Skip to content

[NOJIRA] fix: support expressions in action name attributes (babel) - #1487

Open
marco-saia-datadog wants to merge 5 commits into
developfrom
marco.saia/fix-babel-plugin-action-name-expressions
Open

marco-saia-datadog wants to merge 5 commits into
developfrom
marco.saia/fix-babel-plugin-action-name-expressions

Conversation

@marco-saia-datadog

@marco-saia-datadog marco-saia-datadog commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

Note

Fixes issue #1486

Summary

The Babel plugin previously only recognized dd-action-name, accessibilityLabel, and custom actionNameAttribute values when provided as string literals. Expressions inside JSX braces were ignored, causing action names to fall back to the accessibility label, content, or component name.

This PR adds support for expression-valued attributes and fixes a scope-related ReferenceError in memoized handlers.

Supported values

Value Before After
testID="save" ✅ ✅
testID={"save"}, testID={`save`} ❌ ✅
testID={SAVE_ID} (module constant) ❌ ✅
testID={TestIDs.save} (imported) ❌ ✅
accessibilityLabel={t('save')} ❌ ✅

Implementation

Expressions are evaluated at runtime, not resolved by Babel. The plugin clones the expression into the generated wrapper, where it is evaluated when the element is pressed. This follows the existing behavior for content candidates such as title and label.

Expressions are forwarded only when all referenced bindings are accessible from the generated wrapper (isExpressionInScope).

  • Inline handlers: Expressions can reference local bindings such as .map() parameters. Attributes from nested children with inaccessible bindings are skipped.
  • Memoized handlers (useCallback / useMemo): Arguments are hoisted into the hook call. A separate hoistedDdValues map retains only expressions accessible from the component scope. This excludes .map() parameters and other local bindings. The same scope checks apply to getContent candidates and children.

Memoized handler regression fix

Previously, this pattern could throw a ReferenceError when pressed:

items.map(item => (
  <Button title={item.name} onPress={handlePress} />
))

When handlePress was memoized with useCallback, the plugin hoisted item.name into the hook, where item was out of scope. The error occurred in getContent() before the user's handler executed.

The new scope checks prevent inaccessible expressions from being hoisted.

Runtime name resolution

getTargetName now skips undefined, null, empty strings, and empty arrays. It falls back to the next available source instead of producing "undefined" as the action name.

Limitations

The following behaviors are unchanged:

  • Memoized handlers capture component-scoped values according to their hook dependencies.
  • When multiple elements share a memoized handler, only the first element's values are hoisted.
  • Files without expression-valued DD attributes produce unchanged output. All 123 existing snapshots pass without modifications.

Tests

  • actionNameAttribute.test.ts (new): Executes transformed components with stubbed modules and verifies action names passed to wrapRumAction on press. Covers supported expression types, .map() scopes, nested children, memoized handlers, and the ReferenceError regression.
  • DdBabelInteractionTracking.test.ts: Covers fallback behavior for empty or unresolved values and numeric action names.

@datadog-prod-us1-3

datadog-prod-us1-3 Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Tests

✅ All CI checks and tests passed.

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: bb5632e | Docs | View more details | Give us feedback!

@marco-saia-datadog
marco-saia-datadog force-pushed the marco.saia/fix-babel-plugin-action-name-expressions branch from 5ae0758 to c18f2de Compare October 12, 2026 08:41
`dd-action-name`, `accessibilityLabel` and the custom `actionNameAttribute`
were only collected when written as string literal attributes, so values
like `testID={TestIDs.button}` or `accessibilityLabel={t('save')}` were
silently dropped and the action name fell back to content.

Expressions are now forwarded and evaluated at press time inside the
generated wrapper, as long as all their bindings are visible from where
the wrapper runs. Memoized handlers (`useCallback`/`useMemo`) get a
separate args object that only keeps values visible from the component
body, which also fixes a `ReferenceError` when content candidates or
children referenced `.map()` params.

At runtime, empty or unresolved values are skipped when resolving the
target name instead of producing `"undefined"`.
@marco-saia-datadog
marco-saia-datadog force-pushed the marco.saia/fix-babel-plugin-action-name-expressions branch from c18f2de to 5657daf Compare October 12, 2026 09:33
@marco-saia-datadog
marco-saia-datadog marked this pull request as ready for review October 12, 2026 10:18
Copilot AI balanced review requested due to automatic review settings October 12, 2026 10:18
@marco-saia-datadog
marco-saia-datadog requested a review from a team as a code owner October 12, 2026 10:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Expression-local bindings are rejected and runtime filtering can corrupt compound-component name indexing.

2 open findings
What changed in this PR

Adds runtime support for expression-valued action names while preventing out-of-scope expressions in memoized handlers.

Changes:

  • Collects and scope-checks JSX expressions.
  • Separates inline and hoisted tracking values.
  • Improves runtime fallback and adds regression tests.
File Description
packages/​react-native-babel-plugin/​test/​actionNameAttribute.test.ts Tests expression-valued attributes and memoized handlers.
packages/​react-native-babel-plugin/​src/​utils/​nodeProcessing.ts Adds expression scope validation.
packages/​react-native-babel-plugin/​src/​actions/​rum/​tap.ts Uses scope-safe values for memoized handlers.
packages/​react-native-babel-plugin/​src/​actions/​rum/​index.ts Collects expressions and builds scoped content resolvers.
packages/​core/​src/​rum/​instrumentation/​interactionTracking/​DdBabelInteractionTracking.ts Validates runtime naming values.
packages/​core/​src/​rum/​instrumentation/​interactionTracking/​__tests__/​DdBabelInteractionTracking.test.ts Tests naming fallback behavior.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/react-native-babel-plugin/src/utils/nodeProcessing.ts
- Runtime: resolve the indexed value per naming source instead of filtering
  the arrays first, so compound component positions are preserved. An
  unresolved value at the index falls through to the next source.
- Plugin: `isExpressionInScope` ignores bindings declared within the
  expression itself (e.g. callback params), so values like
  `testID={items.map(item => item.id).join(',')}` and children such as
  `{items.map(i => <Text>{i.name}</Text>)}` are no longer rejected.
Copilot AI balanced review requested due to automatic review settings October 12, 2026 10:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Dropped out-of-scope expressions can shift compound-component positions and produce incorrect action names.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced

Comment thread packages/react-native-babel-plugin/src/actions/rum/index.ts Outdated
`jsxChildToRuntimeCall` only converts JSX it reaches directly. JSX nested in
callbacks (e.g. `{items.map(i => <Text>{i.name}</Text>)}`), non-JSX ternary
branches or attribute values was cloned as-is. Inline, the JSX transform
still picked it up, but code hoisted into `useCallback`/`useMemo` is never
revisited, so raw JSX ended up in the bundle and broke it.

Run a post-pass over getContent children and candidates that replaces any
remaining JSX with jsx()/jsxs() runtime calls.
Copilot AI balanced review requested due to automatic review settings October 12, 2026 11:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Hoisted expressions can retain untransformed TSX syntax or trigger temporal-dead-zone errors in useMemo.

2 open findings

🧠 Review effort: Balanced

Comment thread packages/react-native-babel-plugin/src/actions/rum/index.ts Outdated
- Expressions that are not forwarded now leave a `void 0` placeholder
  instead of being skipped, so compound component positions indexed by
  the handler argument don't shift onto the wrong value.
- `useMemo` factories run during render at the hook call, so component
  bindings declared after it are still in their temporal dead zone. A
  separate args object for `useMemo` only keeps expressions whose
  bindings are program-scoped (imports, top-level declarations).
Copilot AI balanced review requested due to automatic review settings October 12, 2026 11:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The useMemo scope handling can either discard valid component values or evaluate content bindings during their temporal dead zone.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment thread packages/react-native-babel-plugin/src/actions/rum/index.ts Outdated
Comment thread packages/react-native-babel-plugin/src/actions/rum/index.ts
- Program scope only was too strict for `useMemo` factories: params and
  bindings declared before the hook are initialized when the factory runs.
  `isExpressionInScope` now takes an optional position and accepts
  program-scope bindings, params, function declarations, and declarations
  that end before it. The position is the earliest `useMemo` call used by
  the element's handlers.
- `useMemo` gets its own `getContent` resolver with the same check, so
  content candidates or children referencing bindings declared after the
  hook no longer throw during render.
Copilot AI balanced review requested due to automatic review settings October 12, 2026 12:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Nested JSX can escape transformation, and the useMemo tests currently count render-time actions as presses.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment on lines +764 to +770
const expression = data.expression;

const isInline = isExpressionInScope(exprPath, path);
const getValue = (isVisible: boolean) =>
isVisible
? t.cloneNode(expression, true)
: t.unaryExpression('void', t.numericLiteral(0));
Comment on lines +118 to +126
if (typeof exportsObj.render === 'function') {
exportsObj.render();
}

for (const props of rendered) {
props.onPress?.();
}

return { output, captured, contentErrors };

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants