Match RN Text accessibility/disabled behavior + add differential parity tests#23
Merged
Conversation
Renders the real RN wrapper components (oracle) alongside Boost-transformed output, mocks the native host components to capture exact prop bags, and asserts structural equality per Platform.OS. - Add dedicated vitest project (vitest.config.parity.mts) that transforms RN Flow source via @react-native/babel-preset, redirects native/leaf modules to mocks, and dedups React onto the copy react-dom resolves. - Wire unit + parity into a single `pnpm test` via vitest `test.projects`. - Pin react/react-dom@19.2.0 (matching RN 0.83.2) plus babel presets. The 11 known divergences (accessible default + disabled<->accessibilityState reconciliation) are intentionally red; the runtime fixes are a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzGmSHV9mGmFEs1QZ5XmhF
Optimized `<Text>` previously dropped the platform-specific `accessible` default and skipped the `disabled` <-> `accessibilityState.disabled` reconciliation that the RN `Text` wrapper performs, diverging from the native prop bag. - runtime: add `getDefaultTextAccessible()` (render-time `Platform.select`) for the common no-accessibility path; extend `processAccessibilityProps` to reconcile `disabled` with `accessibilityState.disabled`, mirroring Text.js exactly. - plugin: route `disabled` + accessibility props through the runtime helper, and inject the lightweight `accessible` default otherwise. - parity: render the Boost side under the same `Platform.OS` as the wrapper so render-time platform defaults are compared correctly. - tests/fixtures: cover both platforms and the disabled reconciliation; regenerate affected text fixtures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzGmSHV9mGmFEs1QZ5XmhF
Apply code-review feedback on the accessibility/disabled change: - extract the duplicated `NORMALIZED_PROPERTIES` filter into a single `isNormalizedProperty` type guard, used by both the routing and the attribute-skip pass - consolidate the split `react-native-boost` plugin-utils imports No behavior change; all parity/unit fixtures unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzGmSHV9mGmFEs1QZ5XmhF
✅ Deploy Preview for react-native-boost ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Action performedFull review finished. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR adds platform-aware ChangesRuntime accessibility & plugin optimizer
Differential Parity Test Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-native-boost/src/plugin/__tests__/parity/parity.test.ts`:
- Around line 47-58: The test cases in the TEXT_CASES and VIEW_CASES test blocks
silently return when boost.optimized is false, which can hide regressions.
Instead of returning early, add an assertion to explicitly verify that the
bailout is expected or handle it as a test condition. Additionally, the parity
assertion between boost and wrapper should not only compare props but also
assert the `which` property to ensure both the optimization intent and host kind
match correctly. Update both the it.each blocks for 'Text: %s' and 'View: %s' to
include these assertions rather than silently skipping the validation.
In
`@packages/react-native-boost/src/plugin/__tests__/parity/vitest.config.parity.mts`:
- Around line 7-11: The RN_SRC regex pattern uses forward slashes which will not
match Windows file paths containing backslashes from Vite's id parameter. To fix
this, normalize the id parameter from Vite to use forward slashes before
applying it against the RN_SRC regex pattern in the transform hook. You can use
the u function approach or simply replace backslashes with forward slashes in
the id parameter before matching. Apply this same normalization fix to the
transform hooks in wrapper.ts and boost.ts for consistency across all Vite
config files.
In `@packages/react-native-boost/src/plugin/optimizers/text/index.ts`:
- Around line 264-283: The issue is that addDefaultProperty is called after the
attributes array has been rebuilt with spreadAttributes, which contain
unresolvable spreads that cause addDefaultProperty to skip insertion. Move the
call to addDefaultProperty (which injects the accessible default with
getDefaultTextAccessible) to occur before the attributes array is modified with
the spread calls on line 264, ensuring the accessible property is added to the
path before spreads are introduced into the attributes.
- Around line 196-200: The shouldNormalize variable in the text optimizer only
checks for the disabled attribute as a direct JSX attribute using
t.isJSXAttribute, which misses cases where disabled is passed through spread
props like {...{ disabled: true }}. Extend the attribute checking logic to also
detect when disabled might be present in spread attributes (JSXSpreadAttribute),
so that processAccessibilityProps is always called and disabled is properly
reconciled with accessibilityState.disabled regardless of whether it's a direct
attribute or part of a spread object.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5a8086b-2412-4c7c-afcd-06bcf610e8a7
⛔ Files ignored due to path filters (2)
packages/react-native-boost/src/plugin/__tests__/parity/__generated__/.gitignoreis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
packages/react-native-boost/package.jsonpackages/react-native-boost/src/plugin/__tests__/parity/boost.tspackages/react-native-boost/src/plugin/__tests__/parity/capture.tsxpackages/react-native-boost/src/plugin/__tests__/parity/mocks/Platform.tspackages/react-native-boost/src/plugin/__tests__/parity/mocks/PressabilityDebug.tspackages/react-native-boost/src/plugin/__tests__/parity/mocks/ReactNativeFeatureFlags.tspackages/react-native-boost/src/plugin/__tests__/parity/mocks/TextNativeComponent.tspackages/react-native-boost/src/plugin/__tests__/parity/mocks/ViewNativeComponent.tspackages/react-native-boost/src/plugin/__tests__/parity/mocks/processColor.tspackages/react-native-boost/src/plugin/__tests__/parity/mocks/react-native.tspackages/react-native-boost/src/plugin/__tests__/parity/mocks/usePressability.tspackages/react-native-boost/src/plugin/__tests__/parity/parity.test.tspackages/react-native-boost/src/plugin/__tests__/parity/setup.tspackages/react-native-boost/src/plugin/__tests__/parity/vitest.config.parity.mtspackages/react-native-boost/src/plugin/__tests__/parity/wrapper.tspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/basic/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/complex-example/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/default-props/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/expo-router-link-alias-as-child/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/expo-router-link-as-child-false-static/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/expo-router-link-as-child-nested-view/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/expo-router-link-as-child/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/expo-router-link-namespace-as-child/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/flattens-styles-at-runtime/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/force-comment/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/ignore-comment/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/mixed-children-types/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/nested-in-object-with-ignore-comment/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/non-react-native-import/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/number-of-lines/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/pressables/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/resolvable-spread-props/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/text-content-as-explicit-child-prop/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/text-content/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/two-components/output.jspackages/react-native-boost/src/plugin/optimizers/text/__tests__/fixtures/variable-child-no-string/output.jspackages/react-native-boost/src/plugin/optimizers/text/index.tspackages/react-native-boost/src/runtime/__tests__/index.test.tspackages/react-native-boost/src/runtime/index.tspackages/react-native-boost/src/runtime/index.web.tspackages/react-native-boost/vitest.config.ts
Comment on lines
+196
to
+200
| const shouldNormalize = | ||
| hasAccessibilityProperty(path, currentAttributes) || | ||
| currentAttributes.some( | ||
| (attribute) => t.isJSXAttribute(attribute) && t.isJSXIdentifier(attribute.name, { name: 'disabled' }) | ||
| ); |
There was a problem hiding this comment.
disabled in spread props currently bypasses reconciliation.
Line 196 only checks disabled as a direct JSX attribute. Cases like <Text {...{ disabled: true }} /> won’t set shouldNormalize, so processAccessibilityProps is skipped and disabled is not reconciled with accessibilityState.disabled as intended.
Suggested fix
- const shouldNormalize =
- hasAccessibilityProperty(path, currentAttributes) ||
- currentAttributes.some(
- (attribute) => t.isJSXAttribute(attribute) && t.isJSXIdentifier(attribute.name, { name: 'disabled' })
- );
+ const hasDisabledProp = currentAttributes.some((attribute) => {
+ if (t.isJSXAttribute(attribute) && t.isJSXIdentifier(attribute.name, { name: 'disabled' })) return true;
+ if (t.isJSXSpreadAttribute(attribute) && t.isObjectExpression(attribute.argument)) {
+ return attribute.argument.properties.some(
+ (p) =>
+ t.isObjectProperty(p) &&
+ ((t.isIdentifier(p.key) && p.key.name === 'disabled') ||
+ (t.isStringLiteral(p.key) && p.key.value === 'disabled'))
+ );
+ }
+ // Keep conservative behavior for unresolved spreads
+ return t.isJSXSpreadAttribute(attribute) && !t.isObjectExpression(attribute.argument);
+ });
+
+ const shouldNormalize = hasAccessibilityProperty(path, currentAttributes) || hasDisabledProp;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-native-boost/src/plugin/optimizers/text/index.ts` around lines
196 - 200, The shouldNormalize variable in the text optimizer only checks for
the disabled attribute as a direct JSX attribute using t.isJSXAttribute, which
misses cases where disabled is passed through spread props like {...{ disabled:
true }}. Extend the attribute checking logic to also detect when disabled might
be present in spread attributes (JSXSpreadAttribute), so that
processAccessibilityProps is always called and disabled is properly reconciled
with accessibilityState.disabled regardless of whether it's a direct attribute
or part of a spread object.
The common optimized `<Text>` path applied `Text`'s platform-specific `accessible` default via a runtime `getDefaultTextAccessible()` call. Metro (and Expo) bundle per platform and report the target on the Babel caller, so the value is known at compile time. Read it via `api.caller` and inline the literal (`true` on iOS, `false` on Android, omitted on web), dropping the import and call from the hot path. The runtime resolver is kept as a fallback for bundlers that don't expose `caller.platform`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzGmSHV9mGmFEs1QZ5XmhF
On the common (no-a11y) path the `accessible` default was injected with
`addDefaultProperty`, which gives up as soon as it encounters an unresolvable
spread. Once `style` is present the rebuilt attributes contain a
`{...processTextStyle(...)}` call spread, so `addDefaultProperty` bailed and the
default was silently dropped — leaving styled `<Text>` without the platform
`accessible` value RN applies (and a dangling `getDefaultTextAccessible`
import). Build the attribute explicitly and append it instead; the cheap path
guarantees no `accessible` is already set, so this is safe.
Adds styled parity cases (which previously had no coverage) and updates the
flattens-styles fixture that was capturing the buggy output.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HzGmSHV9mGmFEs1QZ5XmhF
The parity it.each blocks only compared prop bags. A bailed Boost output
silently returns `{ optimized: false }` and the test skipped it, so a
regression that stopped optimizing a case (a real loss) would pass as
green. Assert each Text case optimizes, each View case matches its
expected bail status (tracked in BAILED_VIEW_CASES), and that the native
host kind (`which`) agrees with the wrapper oracle.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HzGmSHV9mGmFEs1QZ5XmhF
`new URL(...).pathname` percent-encodes URL-unsafe characters — a space in a parent directory becomes `%20`, so the subsequent `writeFileSync` / dynamic `import` target a path that doesn't exist — and on Windows leaves a stray leading slash before the drive letter (`/C:/...`). `fileURLToPath` decodes both correctly. Affects the parity harness's generated-file paths (boost.ts, wrapper.ts) and the vite config's `u()` path helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzGmSHV9mGmFEs1QZ5XmhF
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.
Adds a differential parity harness that renders real React Native
Text/Viewagainst Boost's optimized output and asserts the native prop bags match per platform. This is the only reliable way to catch where the optimizer silently diverges from RN's wrapper, since those wrappers do non-trivial prop reconciliation before hitting the native host.The harness immediately surfaced two
Textdivergences, which this PR fixes: optimized<Text>dropped the platform-specificaccessibledefault (iOS opts text in, Android defaults off), and it skipped thedisabled↔accessibilityState.disabledreconciliation. The fix mirrorsText.jsexactly — a lightweightgetDefaultTextAccessible()for the common no-a11y path, and the full reconciliation inprocessAccessibilityPropswhen accessibility/disabledprops are present.Both values are resolved at render time (like RN's own
Platform.select) rather than hoisted, so they track the platform correctly. Affected text fixtures are regenerated; all View parity cases were already passing and are unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Bug Fixes
accessibleproperty.disabledprop and accessibility state properties.