diff --git a/.changeset/actions-single-wrap-3962.md b/.changeset/actions-single-wrap-3962.md new file mode 100644 index 000000000..478d41f03 --- /dev/null +++ b/.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. diff --git a/.changeset/people-picker-cursor-render-phase-reset.md b/.changeset/people-picker-cursor-render-phase-reset.md new file mode 100644 index 000000000..8b24fcda2 --- /dev/null +++ b/.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. diff --git a/packages/app-shell/src/utils/__tests__/actionResponse.test.ts b/packages/app-shell/src/utils/__tests__/actionResponse.test.ts index cc4a405e2..c9e022739 100644 --- a/packages/app-shell/src/utils/__tests__/actionResponse.test.ts +++ b/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(); diff --git a/packages/app-shell/src/utils/actionResponse.ts b/packages/app-shell/src/utils/actionResponse.ts index 54a072e1a..ed89c7377 100644 --- a/packages/app-shell/src/utils/actionResponse.ts +++ b/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 diff --git a/packages/fields/src/widgets/PeoplePicker.test.tsx b/packages/fields/src/widgets/PeoplePicker.test.tsx index 36c73cb29..ee0974bb6 100644 --- a/packages/fields/src/widgets/PeoplePicker.test.tsx +++ b/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( + , + ); + 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( diff --git a/packages/fields/src/widgets/PeoplePicker.tsx b/packages/fields/src/widgets/PeoplePicker.tsx index 79b128c8b..76fbd4e20 100644 --- a/packages/fields/src/widgets/PeoplePicker.tsx +++ b/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));