From e958540b0a53604d63afcaebdf028f49f3575b2a Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Mon, 26 Oct 2026 15:06:02 +0700 Subject: [PATCH] fix(app): remove SVG prop and nested button warnings on web (#7915) * fix(app): prevent web SVG prop and nested button warnings Consume Unistyles mapping props at the wrapper boundary and use web accessibility attributes for decorative rings. Register composer track rows as menu items so nested actions stay valid and keyboard selection remains available. * test(app): select archive actions by their menu role --- .../e2e/browser/context-window-meter.spec.ts | 9 + packages/app/e2e/support/helpers/subagents.ts | 6 +- .../src/components/context-window-meter.tsx | 5 +- .../composer/tracks-panel.browser.test.tsx | 22 +++ packages/app/src/composer/tracks.tsx | 8 +- .../git/pull-request-panel/checks-ring.tsx | 6 +- .../app/src/styles/unistyles-props.test.ts | 86 ++++++++++ patches/react-native-unistyles+3.2.4.patch | 157 ++++++++++++++++++ scripts/postinstall-patches.mjs | 3 +- 9 files changed, 293 insertions(+), 9 deletions(-) create mode 100644 packages/app/src/styles/unistyles-props.test.ts diff --git a/packages/app/e2e/browser/context-window-meter.spec.ts b/packages/app/e2e/browser/context-window-meter.spec.ts index 79a572a24b..be0835b116 100644 --- a/packages/app/e2e/browser/context-window-meter.spec.ts +++ b/packages/app/e2e/browser/context-window-meter.spec.ts @@ -42,6 +42,14 @@ const test = base.extend<{ agent: MockAgentSession }>({ }, }); +// The outer meter owns the accessible usage label; its SVG is decorative. +async function expectDecorativeContextRing(meter: Locator): Promise { + const ring = meter.locator("svg"); + await expect(ring).toHaveAttribute("aria-hidden", "true"); + await expect(ring).not.toHaveAttribute("accessibilityElementsHidden"); + await expect(ring).not.toHaveAttribute("importantForAccessibility"); +} + // Where the progress arc is painted, as its centroid relative to the ring's centre in pixels. // Reads the rendered pixels, so any rotation that does not reach the screen counts as none. async function progressArcCentroid(meter: Locator): Promise<{ x: number; y: number }> { @@ -100,6 +108,7 @@ test.describe("context window meter", () => { const meter = page.getByTestId("context-window-meter"); await expect(meter).toHaveAccessibleName(/25%/, { timeout: 30_000 }); + await expectDecorativeContextRing(meter); const centroid = await progressArcCentroid(meter); expect(centroid.x).toBeGreaterThan(1); expect(centroid.y).toBeLessThan(-1); diff --git a/packages/app/e2e/support/helpers/subagents.ts b/packages/app/e2e/support/helpers/subagents.ts index 3354bf3dd8..a6fc5bcf21 100644 --- a/packages/app/e2e/support/helpers/subagents.ts +++ b/packages/app/e2e/support/helpers/subagents.ts @@ -188,7 +188,7 @@ export async function expectManagedSubagentArchived( } export async function archiveFinishedSubagents(page: Page): Promise { - await page.getByRole("button", { name: "Archive finished subagents" }).click(); + await page.getByRole("menuitem", { name: "Archive finished subagents" }).click(); } export async function expectArchiveFinishedInProgress( @@ -196,7 +196,7 @@ export async function expectArchiveFinishedInProgress( completed: number, total: number, ): Promise { - await expect(page.getByRole("button", { name: "Archive finished subagents" })).toBeDisabled(); + await expect(page.getByRole("menuitem", { name: "Archive finished subagents" })).toBeDisabled(); await expect(page.getByText(`${completed}/${total}`, { exact: true })).toBeVisible(); } @@ -206,7 +206,7 @@ export async function expectArchiveFinishedRetry( total: number, ): Promise { await expect(page.getByText(`Retry (${failed}/${total})`, { exact: true })).toBeVisible(); - await expect(page.getByRole("button", { name: "Archive finished subagents" })).toBeEnabled(); + await expect(page.getByRole("menuitem", { name: "Archive finished subagents" })).toBeEnabled(); } export async function holdManagedSubagentArchiveRequest( diff --git a/packages/app/src/components/context-window-meter.tsx b/packages/app/src/components/context-window-meter.tsx index f9cfabcf8d..32c739c073 100644 --- a/packages/app/src/components/context-window-meter.tsx +++ b/packages/app/src/components/context-window-meter.tsx @@ -112,8 +112,9 @@ const ContextWindowRing = withUnistyles(function ContextWindowRing({ width={size} height={size} viewBox={`0 0 ${size} ${size}`} - accessibilityElementsHidden - importantForAccessibility="no-hide-descendants" + {...(isNative + ? { accessibilityElementsHidden: true, importantForAccessibility: "no-hide-descendants" } + : { "aria-hidden": true })} > { ); openPanel(); + expect(document.querySelector("button button")).toBeNull(); + expect(row("row")?.getAttribute("role")).toBe("menuitem"); click(row("row-archive") as Element); expect(onAction).toHaveBeenCalledTimes(1); expect(onPress).not.toHaveBeenCalled(); expect(isPanelOpen()).toBe(true); }); + + it.each(["Enter", " "])("selects a track row with %j", (key) => { + const select = vi.fn(); + mount( + + + Subagent one + + , + ); + openPanel(); + const target = row("row") as HTMLElement; + act(() => { + target.focus(); + target.dispatchEvent(new KeyboardEvent("keydown", { key, bubbles: true })); + target.dispatchEvent(new KeyboardEvent("keyup", { key, bubbles: true })); + }); + expect(select).toHaveBeenCalledTimes(1); + expect(isPanelOpen()).toBe(false); + }); }); describe("composer track pill status mark", () => { diff --git a/packages/app/src/composer/tracks.tsx b/packages/app/src/composer/tracks.tsx index 82bee6e3fa..4e06266058 100644 --- a/packages/app/src/composer/tracks.tsx +++ b/packages/app/src/composer/tracks.tsx @@ -233,6 +233,11 @@ export function ComposerTrackRow({ [closeOnSelect, onPress, selectItem], ); + const itemDataSet = useMemo( + () => ({ menuItem: "true", menuDisabled: disabled ? "true" : "false" }), + [disabled], + ); + const renderRow = useCallback( (active: boolean) => ( @@ -253,7 +258,8 @@ export function ComposerTrackRow({ return ( { + it("consumes uniProps while preserving mapped colors and explicit overrides", async () => { + const result = await renderWebProbe(` + import React from 'react'; + import { renderToStaticMarkup } from 'react-dom/server'; + import { StyleSheet, UnistylesRuntime, withUnistyles } from 'react-native-unistyles'; + import { Server } from 'lucide-react-native'; + StyleSheet.configure({ + themes: { light: { color: 'red' }, dark: { color: 'blue' } }, + settings: { initialTheme: 'light' }, + }); + const Icon = withUnistyles(Server); + const mapping = theme => ({ color: theme.color }); + const render = props => renderToStaticMarkup(React.createElement(Icon, { uniProps: mapping, ...props })); + const light = render(); + UnistylesRuntime.setTheme('dark'); + const dark = render(); + const overridden = render({ color: 'green' }); + const received = []; + const Probe = withUnistyles(props => { received.push(props); return null; }); + renderToStaticMarkup(React.createElement(Probe, { uniProps: mapping, id: 'probe' })); + console.log(JSON.stringify({ light, dark, overridden, keys: Object.keys(received[0]).sort() })); + `); + expect(result.status, result.stderr).toBe(0); + expect(result.stderr).not.toContain("React does not recognize"); + const rendered = JSON.parse(result.stdout); + expect(rendered.keys).toEqual(["color", "id", "ref"]); + expect(rendered.light).toContain('stroke="red"'); + expect(rendered.dark).toContain('stroke="blue"'); + expect(rendered.overridden).toContain('stroke="green"'); + }); +}); + +describe("decorative checks ring", () => { + it("hides its SVG from web accessibility without forwarding native props", async () => { + const result = await renderWebProbe(` + import React from 'react'; + import { renderToStaticMarkup } from 'react-dom/server'; + import { StyleSheet } from 'react-native-unistyles'; + import { ChecksRing } from './src/git/pull-request-panel/checks-ring'; + StyleSheet.configure({ + themes: { light: { colors: { surface3: 'gray', statusSuccess: 'green' } } }, + settings: { initialTheme: 'light' }, + }); + console.log(renderToStaticMarkup(React.createElement(ChecksRing, { + size: 20, summary: { total: 1, parts: [{ status: 'success', count: 1 }] }, + }))); + `); + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toContain('aria-hidden="true"'); + expect(result.stdout).not.toContain("importantForAccessibility"); + expect(result.stdout).not.toContain("accessibilityElementsHidden"); + expect(result.stdout).toContain('stroke="green"'); + expect(result.stderr).not.toContain("React does not recognize"); + }); +}); diff --git a/patches/react-native-unistyles+3.2.4.patch b/patches/react-native-unistyles+3.2.4.patch index 0195868340..7f40053f19 100644 --- a/patches/react-native-unistyles+3.2.4.patch +++ b/patches/react-native-unistyles+3.2.4.patch @@ -329,3 +329,160 @@ index 195954b..c4ac28c 100644 namespace "com.unistyles" defaultConfig { +diff --git a/node_modules/react-native-unistyles/src/core/withUnistyles/withUnistyles.tsx b/node_modules/react-native-unistyles/src/core/withUnistyles/withUnistyles.tsx +--- a/node_modules/react-native-unistyles/src/core/withUnistyles/withUnistyles.tsx ++++ b/node_modules/react-native-unistyles/src/core/withUnistyles/withUnistyles.tsx +@@ -28,13 +28,14 @@ + + return forwardRef, PropsWithUnistyles>((props, ref) => { + const narrowedProps = props as PropsWithUnistyles & UnistyleStyles ++ const { uniProps, ...componentProps } = narrowedProps + const styleClassNames = getClassName(narrowedProps.style, true) + const contentContainerStyleClassNames = getClassName(narrowedProps.contentContainerStyle) + const { proxifiedRuntime, proxifiedTheme } = useProxifiedUnistyles() + + const { key: mappingsKey, ...mappingsProps } = mappings ? mappings(proxifiedTheme, proxifiedRuntime) : {} +- const { key: uniPropsKey, ...unistyleProps } = narrowedProps.uniProps +- ? narrowedProps.uniProps(proxifiedTheme, proxifiedRuntime) ++ const { key: uniPropsKey, ...unistyleProps } = uniProps ++ ? uniProps(proxifiedTheme, proxifiedRuntime) + : {} + + const emptyStyles = narrowedProps.style +@@ -46,7 +47,7 @@ + : undefined + + const combinedProps = { +- ...deepMergeObjects(mappingsProps, unistyleProps, props), ++ ...deepMergeObjects(mappingsProps, unistyleProps, componentProps), + ...(narrowedProps.style + ? { + // Override default component styles with undefined values to reset them +diff --git a/node_modules/react-native-unistyles/src/core/withUnistyles/withUnistyles.native.tsx b/node_modules/react-native-unistyles/src/core/withUnistyles/withUnistyles.native.tsx +--- a/node_modules/react-native-unistyles/src/core/withUnistyles/withUnistyles.native.tsx ++++ b/node_modules/react-native-unistyles/src/core/withUnistyles/withUnistyles.native.tsx +@@ -63,6 +63,7 @@ + + return forwardRef, PropsWithUnistyles>((props, ref) => { + const narrowedProps = props as PropsWithUnistyles & UnistyleStyles ++ const { uniProps, ...componentProps } = narrowedProps + const NativeComponent = Component as ComponentType + + // @ts-ignore we don't know the type of the component +@@ -89,15 +90,15 @@ + }, [narrowedProps.style, narrowedProps.contentContainerStyle]) + + const { key: mappingsKey, ...mappingsProps } = mappings ? mappings(proxifiedTheme, proxifiedRuntime) : {} +- const { key: uniPropsKey, ...unistyleProps } = narrowedProps.uniProps +- ? narrowedProps.uniProps(proxifiedTheme, proxifiedRuntime) ++ const { key: uniPropsKey, ...unistyleProps } = uniProps ++ ? uniProps(proxifiedTheme, proxifiedRuntime) + : {} + + const styleSecrets = getSecrets(narrowedProps.style) + const contentContainerStyleSecrets = getSecrets(narrowedProps.contentContainerStyle) + + const finalProps = { +- ...deepMergeObjects(mappingsProps, unistyleProps, props), ++ ...deepMergeObjects(mappingsProps, unistyleProps, componentProps), + ...(narrowedProps.style + ? { + style: styleSecrets.styles, +diff --git a/node_modules/react-native-unistyles/lib/module/core/withUnistyles/withUnistyles.js b/node_modules/react-native-unistyles/lib/module/core/withUnistyles/withUnistyles.js +--- a/node_modules/react-native-unistyles/lib/module/core/withUnistyles/withUnistyles.js ++++ b/node_modules/react-native-unistyles/lib/module/core/withUnistyles/withUnistyles.js +@@ -13,6 +13,7 @@ + export const withUnistyles = (Component, mappings) => { + return /*#__PURE__*/forwardRef((props, ref) => { + const narrowedProps = props; ++ const { uniProps, ...componentProps } = narrowedProps; + const styleClassNames = getClassName(narrowedProps.style, true); + const contentContainerStyleClassNames = getClassName(narrowedProps.contentContainerStyle); + const { +@@ -26,10 +27,10 @@ + const { + key: uniPropsKey, + ...unistyleProps +- } = narrowedProps.uniProps ? narrowedProps.uniProps(proxifiedTheme, proxifiedRuntime) : {}; ++ } = uniProps ? uniProps(proxifiedTheme, proxifiedRuntime) : {}; + const emptyStyles = narrowedProps.style ? Object.fromEntries(Object.entries(Object.getOwnPropertyDescriptors(narrowedProps.style)).filter(([key]) => !key.startsWith('unistyles') && !key.startsWith('_')).map(([key]) => [key, undefined])) : undefined; + const combinedProps = { +- ...deepMergeObjects(mappingsProps, unistyleProps, props), ++ ...deepMergeObjects(mappingsProps, unistyleProps, componentProps), + ...(narrowedProps.style ? { + // Override default component styles with undefined values to reset them + style: emptyStyles +diff --git a/node_modules/react-native-unistyles/lib/module/core/withUnistyles/withUnistyles.native.js b/node_modules/react-native-unistyles/lib/module/core/withUnistyles/withUnistyles.native.js +--- a/node_modules/react-native-unistyles/lib/module/core/withUnistyles/withUnistyles.native.js ++++ b/node_modules/react-native-unistyles/lib/module/core/withUnistyles/withUnistyles.native.js +@@ -27,6 +27,7 @@ + }; + return /*#__PURE__*/forwardRef((props, ref) => { + const narrowedProps = props; ++ const { uniProps, ...componentProps } = narrowedProps; + const NativeComponent = Component; + + // @ts-ignore we don't know the type of the component +@@ -57,11 +58,11 @@ + const { + key: uniPropsKey, + ...unistyleProps +- } = narrowedProps.uniProps ? narrowedProps.uniProps(proxifiedTheme, proxifiedRuntime) : {}; ++ } = uniProps ? uniProps(proxifiedTheme, proxifiedRuntime) : {}; + const styleSecrets = getSecrets(narrowedProps.style); + const contentContainerStyleSecrets = getSecrets(narrowedProps.contentContainerStyle); + const finalProps = { +- ...deepMergeObjects(mappingsProps, unistyleProps, props), ++ ...deepMergeObjects(mappingsProps, unistyleProps, componentProps), + ...(narrowedProps.style ? { + style: styleSecrets.styles + } : {}), +diff --git a/node_modules/react-native-unistyles/lib/commonjs/core/withUnistyles/withUnistyles.js b/node_modules/react-native-unistyles/lib/commonjs/core/withUnistyles/withUnistyles.js +--- a/node_modules/react-native-unistyles/lib/commonjs/core/withUnistyles/withUnistyles.js ++++ b/node_modules/react-native-unistyles/lib/commonjs/core/withUnistyles/withUnistyles.js +@@ -18,6 +18,7 @@ + const withUnistyles = (Component, mappings) => { + return /*#__PURE__*/(0, _react.forwardRef)((props, ref) => { + const narrowedProps = props; ++ const { uniProps, ...componentProps } = narrowedProps; + const styleClassNames = (0, _getClassname.getClassName)(narrowedProps.style, true); + const contentContainerStyleClassNames = (0, _getClassname.getClassName)(narrowedProps.contentContainerStyle); + const { +@@ -31,10 +32,10 @@ + const { + key: uniPropsKey, + ...unistyleProps +- } = narrowedProps.uniProps ? narrowedProps.uniProps(proxifiedTheme, proxifiedRuntime) : {}; ++ } = uniProps ? uniProps(proxifiedTheme, proxifiedRuntime) : {}; + const emptyStyles = narrowedProps.style ? Object.fromEntries(Object.entries(Object.getOwnPropertyDescriptors(narrowedProps.style)).filter(([key]) => !key.startsWith('unistyles') && !key.startsWith('_')).map(([key]) => [key, undefined])) : undefined; + const combinedProps = { +- ...(0, _utils.deepMergeObjects)(mappingsProps, unistyleProps, props), ++ ...(0, _utils.deepMergeObjects)(mappingsProps, unistyleProps, componentProps), + ...(narrowedProps.style ? { + // Override default component styles with undefined values to reset them + style: emptyStyles +diff --git a/node_modules/react-native-unistyles/lib/commonjs/core/withUnistyles/withUnistyles.native.js b/node_modules/react-native-unistyles/lib/commonjs/core/withUnistyles/withUnistyles.native.js +--- a/node_modules/react-native-unistyles/lib/commonjs/core/withUnistyles/withUnistyles.native.js ++++ b/node_modules/react-native-unistyles/lib/commonjs/core/withUnistyles/withUnistyles.native.js +@@ -32,6 +32,7 @@ + }; + return /*#__PURE__*/(0, _react.forwardRef)((props, ref) => { + const narrowedProps = props; ++ const { uniProps, ...componentProps } = narrowedProps; + const NativeComponent = Component; + + // @ts-ignore we don't know the type of the component +@@ -62,11 +63,11 @@ + const { + key: uniPropsKey, + ...unistyleProps +- } = narrowedProps.uniProps ? narrowedProps.uniProps(proxifiedTheme, proxifiedRuntime) : {}; ++ } = uniProps ? uniProps(proxifiedTheme, proxifiedRuntime) : {}; + const styleSecrets = getSecrets(narrowedProps.style); + const contentContainerStyleSecrets = getSecrets(narrowedProps.contentContainerStyle); + const finalProps = { +- ...(0, _utils.deepMergeObjects)(mappingsProps, unistyleProps, props), ++ ...(0, _utils.deepMergeObjects)(mappingsProps, unistyleProps, componentProps), + ...(narrowedProps.style ? { + style: styleSecrets.styles + } : {}), diff --git a/scripts/postinstall-patches.mjs b/scripts/postinstall-patches.mjs index b009be9841..09ff2919b6 100644 --- a/scripts/postinstall-patches.mjs +++ b/scripts/postinstall-patches.mjs @@ -19,8 +19,9 @@ const patchedPackages = [ patchPrefix: "react-native-enriched-markdown+", cwd: "packages/app", }, - // Remove after react-native-unistyles ships + // Registry cleanup: remove after react-native-unistyles ships // https://github.com/jpudysz/react-native-unistyles/pull/1203. + // withUnistyles: also keep the uniProps filter until upstream consumes its control prop. { nodeModulesPath: "node_modules/react-native-unistyles", patchPrefix: "react-native-unistyles+",