fix(actions): read objectstack#3962's single-wrapped /actions responses (#2971)

Companion to objectstack#3969: /actions failures now speak HTTP and success is
a single wrap — body.data IS the handler's return value.

interpretActionResponse / readActionPayload treat the single wrap as the
primary shape; the pre-#3962 double envelope is detected narrowly
(isLegacyActionEnvelope: boolean success, no keys beyond the envelope's own)
and unwrapped for older runtimes, so a handler value that merely contains a
success key passes through untouched. The 400-rejection shape resolves to a
string toast via actionErrorDetail (no React #31 regression). ActionResult.data's
depth quirk self-heals on #3962 servers.

Also fixes the PeoplePicker keyboard flake at the root: the cursor reset on new
results lived in a useEffect, which flushes asynchronously — a reset queued by
the records arriving could land AFTER a subsequent ArrowDown and wipe the
just-set cursor (the earlier signature-keyed fix closed the too-often resets;
this was the too-late one). The reset now runs in the render phase ("adjusting
state during render"), same render that shows the new rows, so it can never
race a keypress. Semantics pinned by a new test; LookupField's length-keyed
effect reset is deliberately unchanged (different semantics).

Author: os-zhuang

.changeset/actions-single-wrap-3962.md

@@ -0,0 +1,14 @@
+---
+"@object-ui/app-shell": patch
+---
+
+fix(actions): read objectstack#3962's single-wrapped /actions responses; legacy double wrap detected narrowly
+
+objectstack#3962 made `/actions` failures speak HTTP (400 rejection / 404 / 403
+/ 503 / 500) and single-wrapped success — `body.data` IS the handler's return
+value. `interpretActionResponse` / `readActionPayload` now treat that as the
+primary shape: the pre-#3962 double envelope is detected NARROWLY (a boolean
+`success` and no keys beyond the envelope's own) and unwrapped for older
+runtimes, so a handler value that merely contains a `success` key is
+handler-owned and passes through untouched. `ActionResult.data`'s depth quirk
+self-heals on #3962 servers.

.changeset/people-picker-cursor-render-phase-reset.md

@@ -0,0 +1,19 @@
+---
+"@object-ui/fields": patch
+---
+
+fix(fields): PeoplePicker's keyboard cursor can no longer be eaten by a late reset
+
+The cursor reset on new results lived in a `useEffect`. Effects flush
+asynchronously after the render that delivered the records — so a reset queued
+by their arrival could land AFTER a subsequent ArrowDown and wipe the just-set
+cursor. That was the residual ArrowDown→Enter flake in
+`PeoplePicker.test.tsx` (the earlier signature-keyed fix closed the
+too-often resets, not the too-late one), and a real fast-fingers UX bug: rows
+appear, the user presses ArrowDown, the highlight vanishes.
+
+The reset now runs in the render phase (the "adjusting state during render"
+pattern), in the same render that shows the new rows — by the time a row is
+visible, the reset has already happened, so it can never race a keypress.
+Semantics unchanged and now pinned by a test: a replaced result set does not
+inherit the previous set's cursor.

packages/app-shell/src/utils/__tests__/actionResponse.test.ts

@@ -12,10 +12,11 @@ import { interpretActionResponse, readActionPayload } from '../actionResponse';
 const ok = { ok: true, status: 200 };
 
 describe('interpretActionResponse — failure shapes', () => {
-    it('catches a business rejection hiding under HTTP 200 (the reported bug)', () => {
-        // `res.ok` is TRUE and the OUTER `success` is TRUE. Only the inner
-        // envelope reports the failure — miss it and the ActionRunner fires a
-        // green "completed" toast over a failed action.
+    it('catches a LEGACY business rejection hiding under HTTP 200 (the reported bug)', () => {
+        // Pre-objectstack#3962 servers only: `res.ok` is TRUE and the OUTER
+        // `success` is TRUE. Current servers answer 400, which `res.ok`
+        // catches — this branch keeps the console honest against older
+        // runtimes.
         const out = interpretActionResponse(ok, {
             success: true,
             data: { success: false, error: "Action 'log_call' on object '*' not found" },
@@ -25,6 +26,19 @@ describe('interpretActionResponse — failure shapes', () => {
         expect(out.error).toBe("Action 'log_call' on object '*' not found");
     });
 
+    it('catches a 400 rejection (#3962) and resolves the nested {message} to a STRING', () => {
+        const rejection = interpretActionResponse({ ok: false, status: 400 }, {
+            success: false,
+            error: {
+                message: 'ValidationError: issued_on is required',
+                code: 400,
+                details: { code: 'VALIDATION_FAILED', fields: [{ field: 'issued_on' }] },
+            },
+        }, 'Action "submit_signoff"');
+        expect(rejection.ok).toBe(false);
+        expect(rejection.error).toBe('ValidationError: issued_on is required');
+    });
+
     it('catches a dispatch failure and resolves the nested {message} to a STRING', () => {
         // Handing the `{message, code}` OBJECT to toast.error() as a React
         // child crashes the page (React #31).
@@ -66,18 +80,24 @@ describe('interpretActionResponse — success', () => {
     });
 });
 
-describe('readActionPayload — the double-wrap trap', () => {
-    it('reaches the handler value through BOTH envelopes', () => {
-        // The bug: reading one level lands on `{success, data}`, where only
-        // `success` and `data` ever live — so an action returning
-        // `{ redirectUrl }` was silently ignored, because
-        // `body.data.redirectUrl` is never set by anything.
+describe('readActionPayload — legacy double wrap vs #3962 single wrap', () => {
+    it('unwraps the LEGACY double envelope to reach the handler value', () => {
+        // Pre-#3962 servers double-wrapped; an action returning
+        // `{ redirectUrl }` lived one level below where every reader looked.
         const envelope = { success: true, data: { redirectUrl: 'https://example.test/sso' } };
 
         expect((envelope as any).redirectUrl).toBeUndefined();       // the old read
         expect(readActionPayload(envelope)).toEqual({ redirectUrl: 'https://example.test/sso' });
     });
 
+    it('passes a #3962 single-wrapped handler value through — even one with success-ish keys', () => {
+        // Current servers put the handler value directly in `body.data`. Only
+        // the EXACT legacy shape is unwrapped; a handler value that merely
+        // contains a boolean `success` plus its own keys is handler-owned.
+        const payload = { success: true, rows: 3, data: { id: 'x' }, extra: 'mine' };
+        expect(readActionPayload(payload)).toEqual(payload);
+    });
+
     it('passes a non-enveloped body through rather than inventing undefined', () => {
         expect(readActionPayload({ id: 'x' })).toEqual({ id: 'x' });
         expect(readActionPayload(null)).toBeNull();

packages/app-shell/src/utils/actionResponse.ts

@@ -8,7 +8,7 @@
  * action fired a green "completed" toast on every list/page surface. Two copies
  * of a subtle envelope rule is a bug generator; this is the rule, once.
  *
- * ## The envelope is DOUBLE, and that is the trap
+ * ## The legacy envelope was DOUBLE, and that was the trap (pre-objectstack#3962)
  *
  * ```
  * {                          ← transport envelope
@@ -62,14 +62,26 @@ export interface ActionResponseOutcome {
  * that level is constructed by the server and only ever holds `success`/`data`.
  */
 export function readActionPayload(envelope: unknown): unknown {
-    if (envelope && typeof envelope === 'object' && !Array.isArray(envelope) && 'success' in envelope) {
+    // objectstack#3962 servers single-wrap: `body.data` IS the handler value,
+    // so most of the time this is the identity function. Only the exact LEGACY
+    // action envelope (pre-#3962: a boolean `success` and no keys beyond the
+    // envelope's own) is unwrapped one more level — a handler value that
+    // merely contains a `success` key passes through untouched.
+    if (isLegacyActionEnvelope(envelope)) {
         return (envelope as { data?: unknown }).data;
     }
-    // A server that did not double-wrap (or a stubbed response in a test):
-    // treat what we have as the payload rather than inventing an undefined.
     return envelope;
 }
 
+const LEGACY_ENVELOPE_KEYS = new Set(['success', 'data', 'error', 'code', 'fields']);
+
+/** The pre-objectstack#3962 inner envelope, detected narrowly. */
+export function isLegacyActionEnvelope(v: unknown): boolean {
+    return !!v && typeof v === 'object' && !Array.isArray(v)
+        && typeof (v as any).success === 'boolean'
+        && Object.keys(v).every((k) => LEGACY_ENVELOPE_KEYS.has(k));
+}
+
 /**
  * Classify an `/actions` response. `label` names the action for the fallback
  * message (e.g. `Action "log_call" failed`).
@@ -82,7 +94,11 @@ export function interpretActionResponse(
     label: string,
 ): ActionResponseOutcome {
     const envelope = json?.data;
-    const innerFailed = !!envelope && typeof envelope === 'object' && (envelope as any).success === false;
+    // Pre-#3962 servers reported a rejection as HTTP 200 with the inner
+    // `{success:false, error}`; current servers answer a real status, which
+    // `res.ok` catches first. Detected narrowly so a handler value that merely
+    // contains `success: false` is not misread as a failure.
+    const innerFailed = isLegacyActionEnvelope(envelope) && (envelope as any).success === false;
 
     if (!res.ok || (json && json.success === false) || innerFailed) {
         // A business rejection carries its message on the INNER envelope; a

packages/fields/src/widgets/PeoplePicker.test.tsx

@@ -189,6 +189,26 @@ describe('PeoplePicker', () => {
     expect(onSelect).toHaveBeenCalledWith('u1');
   });
 
+  it('keyboard: cursor resets when the results are replaced by a search', async () => {
+    // Pins the semantics the render-phase reset must preserve: a NEW result
+    // set does not inherit the previous set's cursor. (The reset moved out of
+    // an effect into the render phase so it can never land AFTER a subsequent
+    // ArrowDown — the CI flake on the test above.)
+    const ds = makeDataSource();
+    render(
+      <PeoplePicker {...baseProps} dataSource={ds} onSelect={vi.fn()} onSelectRecords={vi.fn()} onOpenChange={vi.fn()} />,
+    );
+    await waitFor(() => expect(screen.getByText('Amy Lin')).toBeTruthy());
+    const search = screen.getByTestId('people-picker-search');
+    fireEvent.keyDown(search, { key: 'ArrowDown' });
+    await waitFor(() =>
+      expect(screen.getAllByTestId('person-row')[0].getAttribute('data-active')).toBe('true'),
+    );
+    fireEvent.change(search, { target: { value: 'bob' } });
+    await waitFor(() => expect(screen.getAllByTestId('person-row')).toHaveLength(1));
+    expect(screen.getAllByTestId('person-row')[0].getAttribute('data-active')).not.toBe('true');
+  });
+
   it('keyboard: Backspace on empty search removes the last chip (multi)', async () => {
     const ds = makeDataSource();
     render(

packages/fields/src/widgets/PeoplePicker.tsx

@@ -320,10 +320,22 @@ export function PeoplePicker({
     () => query.records.map(r => String(getPersonId(r, idField))).join(''),
     [query.records, idField],
   );
-  useEffect(() => {
+  // The reset runs in the RENDER PHASE (the "adjusting state during render"
+  // pattern), NOT in an effect. An effect flushes asynchronously after the
+  // render that delivered the records — so a reset queued by their arrival
+  // could land AFTER a subsequent ArrowDown and eat the just-set cursor.
+  // That is the residual ArrowDown→Enter flake the signature key above did
+  // not close (CI reproduced it under load), and a real fast-fingers UX bug:
+  // rows appear, the user presses ArrowDown, the highlight vanishes.
+  // Resetting during the same render that shows the new rows makes the
+  // ordering deterministic — by the time the rows are visible, the reset
+  // has already happened.
+  const cursorEpoch = `${query.search}\u0000${recordsSignature}`;
+  const [seenCursorEpoch, setSeenCursorEpoch] = useState(cursorEpoch);
+  if (seenCursorEpoch !== cursorEpoch) {
+    setSeenCursorEpoch(cursorEpoch);
     setActiveIndex(-1);
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [query.search, recordsSignature]);
+  }
   // Keep it in range if the list shrinks.
   useEffect(() => {
     setActiveIndex(i => (i >= navList.length ? navList.length - 1 : i));