fix(app-shell): every CelPredicateField binds its label — useId fallback when no id is passed (#4533)

.changeset/cel-predicate-field-label-binding.md

@@ -0,0 +1,23 @@
+---
+'@object-ui/app-shell': patch
+---
+
+fix(app-shell): every CelPredicateField binds its label, so the RLS row-filter editors have an accessible name
+
+`CelPredicateField` renders both halves of its own label association — the
+`Label htmlFor` and the `Textarea id` — but `id` was an optional prop with no
+fallback, so a call site that omitted it left both `undefined`: a label that
+read correctly on screen and resolved to nothing, and an editor with no
+accessible name.
+
+The row-level-security `USING (read filter)` and `CHECK (write filter)` clauses
+on the permission-set editor were mounted that way, which made the row-filter
+authoring surface unreachable by label for screen readers and for
+`getByLabelText`. The condition builder's CEL escape hatch was affected too.
+
+`id` now falls back to a generated `useId()`, so the association is a property
+of mounting the component rather than of every caller remembering the prop. An
+explicit `id` still wins unchanged — the field inspector's four editors and the
+conditional-formatting editor keep the exact ids they already had. The
+autocomplete listbox is named off the same resolved id, so an id-less mount now
+also carries the `aria-controls` that ties the combobox to its popup.

packages/app-shell/src/views/metadata-admin/CelPredicateField.labelBinding.test.tsx

@@ -0,0 +1,274 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * objectui#4533 — every `CelPredicateField` mount must bind its own label.
+ *
+ * The component renders BOTH halves of the association itself:
+ * `Label htmlFor={id}` and `Textarea id={id}`. While `id` was an optional prop
+ * with no fallback, a call site that omitted it produced `undefined` on both —
+ * a label that reads correctly on screen and resolves to nothing, so the
+ * textarea has no accessible name at all.
+ *
+ * The card surfaced from the authoring side: writing objectui#4302's wire pins,
+ * the obvious selector was refused verbatim by testing-library —
+ *
+ *   TestingLibraryElementError: Found a label with the text of:
+ *   USING (read filter), however no form control was found associated to that
+ *   label.
+ *
+ * — and that test had to select the RLS editor by placeholder instead
+ * (`PermissionMatrixEditor.packageDoorFacets.test.tsx`, whose comment points
+ * here). So the query below is not a proxy for the defect; it IS the refused
+ * query, and the row-filter authoring surface of a permission set is the
+ * surface it was refused on.
+ *
+ * The fix is at the COMPONENT, not the call sites: `id` falls back to
+ * `React.useId()`, so the association is a property of mounting the component
+ * rather than of every caller remembering a prop. That is what these pins are
+ * shaped to prove — the id-less cases are the red-first, and the explicit-id
+ * cases pin that a passed `id` still wins unchanged, since four call sites
+ * depend on their exact ids.
+ *
+ * Two call sites the card's table did not have right, both measured on
+ * `origin/main` before this suite was written:
+ *
+ *  - `ConditionalFormattingEditor` was listed as omitting `id`; it does not,
+ *    and never has (it has passed `cf-condition-${i}` since #2558, and four
+ *    assertions in `ConditionalFormattingEditor.test.tsx` select on that id).
+ *    The card recorded the mount's line number as the defect. Pinned below as
+ *    an explicit-id site.
+ *  - `inspectors/ConditionBuilder.tsx` DOES omit `id` and was not in the table.
+ *    It is owned by another in-flight change and is not edited here — the
+ *    component-level fallback repairs it without a call-site edit, which is
+ *    precisely the argument for fixing the contract instead of the callers.
+ */
+
+import '@testing-library/jest-dom/vitest';
+import * as React from 'react';
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import { render, screen, fireEvent, cleanup, waitFor } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+
+vi.mock('./useMetadata', () => ({
+  useMetadataClient: () => ({
+    list: vi.fn().mockResolvedValue([]),
+    listDrafts: vi.fn().mockResolvedValue([]),
+  }),
+}));
+
+vi.mock('./previews/useObjectFields', () => ({
+  useObjectFields: () => ({ fields: [], loading: false, error: null }),
+}));
+
+import { CelPredicateField } from './CelPredicateField';
+import { PermissionAdvancedFacets } from './PermissionAdvancedFacets';
+import { ConditionalFormattingEditor } from './ConditionalFormattingEditor';
+import { ObjectFieldInspector } from './inspectors/ObjectFieldInspector';
+import { __setCelFormulaLoader } from './celAuthoring';
+import { t as translate } from './i18n';
+
+afterEach(() => {
+  cleanup();
+  __setCelFormulaLoader(undefined);
+});
+
+/** Real en-US strings, so the labels below are the ones an author reads. */
+const t = (k: string) => translate(k, 'en-US');
+
+/** The exact label text testing-library refused to resolve (i18n `perm.rls.using`). */
+const USING_LABEL = 'USING (read filter)';
+const CHECK_LABEL = 'CHECK (write filter)';
+
+/** Controlled harness — the field is controlled, so hold its value in state. */
+function Harness({ initial = '', ...rest }: { initial?: string } & Record<string, unknown>) {
+  const [v, setV] = React.useState(initial);
+  return (
+    <CelPredicateField
+      value={v}
+      onChange={setV}
+      label={USING_LABEL}
+      objectName="account"
+      fieldNames={['organization_id', 'owner_id']}
+      clause="using"
+      t={t}
+      {...rest}
+    />
+  );
+}
+
+function renderFacets(over: Record<string, unknown> = {}) {
+  const props = {
+    draft: {
+      rowLevelSecurity: [
+        { name: 'p1', object: 'account', operation: 'all', using: '', check: '', enabled: true },
+      ],
+    },
+    setDraft: () => {},
+    writable: true,
+    allSetNames: [] as string[],
+    loadObjectFields: async () => ['organization_id', 'owner_id'],
+    t,
+    ...over,
+  };
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  return render(<PermissionAdvancedFacets {...(props as any)} />);
+}
+
+/** The RLS facet is collapsed by default — expand it to mount the CEL editors. */
+async function openRls() {
+  fireEvent.click(screen.getByText(t('perm.rls.title')));
+  await screen.findAllByPlaceholderText('organization_id == current_user.organization_id');
+}
+
+describe('CelPredicateField · binds its label with no `id` prop (objectui#4533)', () => {
+  it('resolves the accessibility query that was refused, with no `id` passed', () => {
+    render(<Harness />);
+    // The refused query itself. Before the fallback this threw
+    // "no form control was found associated to that label".
+    const box = screen.getByLabelText(USING_LABEL);
+    expect(box.tagName).toBe('TEXTAREA');
+    expect(box).toHaveAccessibleName(USING_LABEL);
+  });
+
+  it('gives each id-less mount its OWN id, so labels do not cross-bind', () => {
+    render(
+      <>
+        <Harness label={USING_LABEL} />
+        <Harness label={CHECK_LABEL} clause="check" />
+      </>,
+    );
+    const using = screen.getByLabelText(USING_LABEL);
+    const check = screen.getByLabelText(CHECK_LABEL);
+    expect(using.id).toBeTruthy();
+    expect(check.id).toBeTruthy();
+    // A constant fallback id would bind both labels to the first control and
+    // still pass the single-mount case above.
+    expect(using.id).not.toBe(check.id);
+    expect(using).not.toBe(check);
+  });
+
+  it('names the autocomplete listbox off the same resolved id', async () => {
+    __setCelFormulaLoader(() =>
+      Promise.resolve({
+        validateExpression: () => ({ ok: true, errors: [], warnings: [] }),
+        introspectScope: () => ({
+          fields: ['organization_id', 'owner_id'],
+          roots: ['current_user'],
+          functions: ['has'],
+        }),
+      }),
+    );
+    const user = userEvent.setup();
+    render(<Harness />);
+    const box = screen.getByLabelText(USING_LABEL);
+    // Typed through userEvent so the caret moves — `refreshAc` reads
+    // `selectionStart` to find the token under it.
+    await user.click(box);
+    await user.type(box, 'org');
+    const list = await screen.findByRole('listbox', {}, { timeout: 3000 });
+    // `aria-controls` was unreachable while the id was undefined: the menu had
+    // no id to point at, so the combobox never announced its own popup.
+    await waitFor(() => expect(box).toHaveAttribute('aria-controls', list.id));
+    expect(list.id).toBeTruthy();
+  });
+});
+
+describe('RLS policy editors bind their labels (objectui#4533 · the card surface)', () => {
+  it('resolves USING by its label — the query objectui#4302 was refused', async () => {
+    renderFacets();
+    await openRls();
+    const box = screen.getByLabelText(USING_LABEL);
+    expect(box.tagName).toBe('TEXTAREA');
+    expect(box).toHaveAttribute('placeholder', 'organization_id == current_user.organization_id');
+  });
+
+  it('resolves CHECK by its label', async () => {
+    renderFacets();
+    await openRls();
+    const box = screen.getByLabelText(CHECK_LABEL);
+    expect(box.tagName).toBe('TEXTAREA');
+    expect(box).toHaveAccessibleName(CHECK_LABEL);
+  });
+
+  it('binds every clause of every policy to its own editor', async () => {
+    renderFacets({
+      draft: {
+        rowLevelSecurity: [
+          { name: 'p1', object: 'account', operation: 'read', using: '', check: '', enabled: true },
+          { name: 'p2', object: 'account', operation: 'write', using: '', check: '', enabled: true },
+        ],
+      },
+    });
+    await openRls();
+    const using = screen.getAllByLabelText(USING_LABEL);
+    const check = screen.getAllByLabelText(CHECK_LABEL);
+    expect(using).toHaveLength(2);
+    expect(check).toHaveLength(2);
+    const ids = [...using, ...check].map((el) => el.id);
+    expect(new Set(ids).size).toBe(4);
+  });
+});
+
+describe('an explicit `id` still wins unchanged (objectui#4533 · pins)', () => {
+  it.each([
+    'field-formula-total',
+    'field-rule-visible-total',
+    'field-rule-readonly-total',
+    'field-rule-required-total',
+    'cf-condition-0',
+  ])('renders %s verbatim on the textarea and on the label', (id) => {
+    render(<Harness id={id} />);
+    const box = screen.getByLabelText(USING_LABEL);
+    expect(box).toHaveAttribute('id', id);
+    expect(document.getElementById(id)).toBe(box);
+  });
+
+  it("keeps ObjectFieldInspector's four call-site ids exactly as they were", async () => {
+    render(
+      <ObjectFieldInspector
+        type="object"
+        name="account"
+        draft={{ name: 'account', fields: { total: { type: 'formula' } } }}
+        selection={{ kind: 'field', id: 'total' }}
+        onPatch={() => {}}
+        onClearSelection={() => {}}
+        onSelectionChange={() => {}}
+        readOnly={false}
+        locale={'en-US'}
+      />,
+    );
+    for (const id of [
+      'field-formula-total',
+      'field-rule-visible-total',
+      'field-rule-readonly-total',
+      'field-rule-required-total',
+    ]) {
+      const box = await waitFor(() => {
+        const el = document.getElementById(id);
+        expect(el).toBeTruthy();
+        return el as HTMLElement;
+      });
+      expect(box.tagName).toBe('TEXTAREA');
+      // The fallback must not have displaced the explicit id.
+      expect(box.id).toBe(id);
+    }
+  });
+
+  it("keeps ConditionalFormattingEditor's cf-condition-{i} id", async () => {
+    render(
+      <ConditionalFormattingEditor
+        rules={[{ condition: '', style: {} }]}
+        onChange={() => {}}
+        objectName="account"
+        fieldNames={['status']}
+        t={t}
+      />,
+    );
+    const box = await waitFor(() => {
+      const el = document.getElementById('cf-condition-0');
+      expect(el).toBeTruthy();
+      return el as HTMLElement;
+    });
+    expect(box.tagName).toBe('TEXTAREA');
+  });
+});

packages/app-shell/src/views/metadata-admin/CelPredicateField.tsx

@@ -124,6 +124,22 @@ export function CelPredicateField({
   t,
   id,
 }: CelPredicateFieldProps) {
+  /**
+   * This component renders BOTH halves of the label association itself — the
+   * `Label htmlFor` and the `Textarea id` below — so binding them is its own
+   * guarantee, not something every call site has to remember (objectui#4533).
+   *
+   * While `id` was optional with no fallback, omitting it left both `undefined`:
+   * the label read correctly on screen and resolved to nothing, so the editor
+   * had no accessible name. The RLS `USING` / `CHECK` clauses — the row-filter
+   * authoring surface of a permission set — were mounted that way, and
+   * `getByLabelText` refused them outright.
+   *
+   * An explicit `id` still wins: four inspector call sites and the
+   * conditional-formatting editor address their editors by exact id.
+   */
+  const fallbackId = React.useId();
+  const fieldId = id ?? fallbackId;
   const taRef = React.useRef<HTMLTextAreaElement>(null);
   const [issues, setIssues] = React.useState<CelLintIssue[]>([]);
   const [linted, setLinted] = React.useState(false);
@@ -304,17 +320,19 @@ export function CelPredicateField({
   // even alongside warnings — the type is what dataset measure eligibility
   // keys off, so the author should see it whenever it is known.
   const showType = role === 'value' && linted && !!value.trim() && errors.length === 0 && inferredType !== null;
-  const listId = id ? `${id}-ac` : undefined;
+  // Derived from the resolved id, so the combobox can name its own popup even
+  // when the call site passed no id (objectui#4533 — same association hole).
+  const listId = `${fieldId}-ac`;
 
   return (
     <div>
-      <Label className="text-[10px] uppercase text-muted-foreground" htmlFor={id}>
+      <Label className="text-[10px] uppercase text-muted-foreground" htmlFor={fieldId}>
         {label}
       </Label>
       <div className="relative">
         <Textarea
           ref={taRef}
-          id={id}
+          id={fieldId}
           value={value}
           disabled={disabled}
           rows={2}