Skip to content
This repository was archived by the owner on Oct 26, 2026. It is now read-only.

Commit 1f8ef0a

Browse files
objectstack-fleet[bot]claudeos-project-manager
authored
fix(fields): report a failed ImageField upload instead of leaking the rejection (#10310)
Fixes #10226 Clause-②: no ## What changed `ImageField` (`@object-ui/fields`) has two upload paths: the native picker (`handleFileChange`) and the crop dialog's confirm (`handleCropConfirm`). On `main` at `8b1f066` both ran the upload inside `try … finally` with **no `catch`**. Both handlers are fire-and-forget: React ignores the promise an `onChange` handler returns, and `ImageCropperDialog` calls `onConfirm(blob, name)` without awaiting it. So a rejected upload (network error, 413, storage outage) showed nothing and escaped as an unhandled promise rejection. This PR catches the failure on both paths and renders the same message `FileField` renders for the same failure through the same `useUpload()` transport: key `fields.file.uploadFailed`, same `name` / `error` arguments, same `defaultValue`. The message goes in the widget's existing error row (the one the oversize-pick rejections already use, objectui#4141). - **Picker path:** each pick is caught on its own, as in `FileField`'s `useFileUploads`. A failed pick is reported and not added. In a multi-select, the picks that did upload still land, and the oversize rejections are kept beside the upload failures. - **Crop path:** the original image stays (no `onChange`). The dialog still closes in `finally`, so the message shows in the field's error row instead of behind the dialog. - The message is a module-level helper, `uploadFailedMessage(t, name, err)`, in the same shape as `maxSizeError`. A component-scoped `useCallback` made the React Compiler skip one more memoization, so the helper lives outside the component. The key exists in every locale pack `@object-ui/i18n` ships. `all-locales-key-parity.test.ts` re-derives that, and `pnpm check:i18n-keys` checks the call site's arguments and inline `defaultValue` against the `en` pack. Files: `packages/fields/src/widgets/ImageField.tsx`, `packages/fields/src/widgets/ImageField.uploadFailure.test.tsx` (new), `.changeset/10226-image-upload-failure-reported.md` (patch, `@object-ui/fields`). ## Evidence (head `737663e`) **Red first, on unmodified source.** I ran the new test against `8b1f066` before touching the widget. All three failure cases went red, and each saw exactly one `unhandledRejection` carrying `network down`, with no failure text in the DOM: ``` × picker: shows the translated failure, adds nothing, leaks no rejection × picker (multiple): adds the successful picks and reports only the failed one × crop: shows the translated failure, keeps the original image, leaks no rejection AssertionError: expected [ Error: network down ] to have a length of +0 but got 1 (x3) TestingLibraryElementError: Unable to find an element with the text: Failed to upload "bad.png": network down Tests 3 failed | 2 passed (5) ``` The two success controls (picker and crop, resolving transport) pass on both trees. **Ablation on the committed fix (head `737663e`).** A trap-guarded script replaced `ImageField.tsx` with its `8b1f066` blob, ran the test, and restored the file with `git checkout HEAD -- PATH`. - On-disk proof of the mutation: `uploadFailed` occurrences went 4 → 0 and `catch (err)` went to 0. - Result on the mutated tree: `Tests 3 failed | 2 passed (5)`, the same three failures as above. - Restore proof: the file's hash equals its HEAD blob, and `git diff HEAD` is empty. **Local gates, on head `737663e`, exit codes written to files before being read:** - `pnpm --filter @object-ui/fields type-check` (`tsc --noEmit && tsc -p tsconfig.test.json`) → exit 0. I built the dependency closure first with `pnpm --filter '@object-ui/fields^...' build`. - `pnpm exec vitest run packages/fields/` → exit 0, `Test Files 179 passed | 1 skipped (180)`, `Tests 3010 passed | 7 skipped (3017)`. - `eslint` on the two touched `.ts(x)` files → 0 errors. `ImageField.tsx` has 7 warnings against 6 on `8b1f066`; the one new warning is below. `lint.yml` sets no `--max-warnings`. - `check:i18n-keys`, `check:i18n-dead-keys`, `check:vi-mock-specifiers`, `check:vi-mock-inherit`, `check:vi-mock-override-shape`, `check:new-line-citations`, `check:control-bytes`, `check:test-path-roots`, `check:unreferenced-sources`, `scripts/check-changeset-presence.mjs`, `scripts/check-changeset-no-major.mjs` → all exit 0. **Scope of local runs:** repo-wide `pnpm lint`, the full `pnpm test`, and every other `check:*` family were left to CI. I ran lint only on the touched files. That narrowing is not a proof that no other file's verdict can move, and I did not check whether type-aware rules are on in `eslint.config.js`. ## Acceptance notes - **One new lint warning, not fixed here.** `react-hooks/preserve-manual-memoization` now also fires on `handleCropConfirm`. Adding the `catch` makes the React Compiler skip memoizing that callback; it already skipped `openCropper` on `main`. This only affects optimization, and the file already carries the same warning. `lint.yml` sets no `--max-warnings`. - **Non-`Error` rejections read `undefined`.** For a rejection that is not an `Error`, `(err as Error).message` puts `undefined` in the message. `FileField` does exactly the same, and I copied its behaviour on purpose so the two widgets say the same thing. Carrier: none. - **Possible shared helper.** `FileField` and `ImageField` now each build the `fields.file.uploadFailed` call the same way. `file-size-guard.ts` explains why the size guard is shared rather than copied per widget, and the same argument could apply to this message. Moving it would touch `FileField.tsx`, which is outside this claim's file surface. Carrier: none. - **Title strings in the crop dialog are hard-coded English.** `ImageCropperDialog`'s title and description do not go through `t()`. I left them alone: they are not part of this defect and not in scope. - **Commit trailer.** The first commit (`2091538`) has a `Co-Authored-By` trailer that names a model version. The later commits use the plain `Claude` form. History was not rewritten. Session: `https://claude.ai/code/session_01D486C1axmXnrkJMNUfz2eb` --- _Generated by [Claude Code](https://claude.ai/code/session_01D486C1axmXnrkJMNUfz2eb)_ --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Leehom <pm@objectstack.ai>
1 parent 687353f commit 1f8ef0a

3 files changed

Lines changed: 228 additions & 8 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@object-ui/fields': patch
3+
---
4+
5+
fix(fields): a failed image upload is reported in `ImageField`, not swallowed
6+
7+
Both upload paths of `ImageField` — the native picker and the crop dialog's
8+
confirm — ran the upload inside `try … finally` with no `catch`. Both are
9+
invoked fire-and-forget (React ignores the promise an `onChange` handler returns,
10+
and the cropper does not await `onConfirm`), so a rejected upload (a network
11+
error, a 413, a storage outage) showed the user nothing and escaped as an
12+
unhandled promise rejection.
13+
14+
A failed upload now renders the same translated `fields.file.uploadFailed`
15+
message `FileField` shows for the same failure, in the widget's existing error
16+
row. The failed image is not added: on the picker path the other picks of a
17+
multi-select still upload and land, as in `FileField`; on the crop path the
18+
original image stays and the dialog closes so the message is visible.

‎packages/fields/src/widgets/ImageField.tsx‎

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,22 @@ const ImageCropperDialog = lazy(() =>
2323
import('./ImageCropperDialog.js').then((m) => ({ default: m.ImageCropperDialog })),
2424
);
2525

26+
/**
27+
* The message for a failed upload — the same translated key, arguments and
28+
* fallback FileField renders for the same failure through the same
29+
* `useUpload()` transport (objectui#10226). Both upload paths in `ImageField`
30+
* are invoked fire-and-forget (React ignores the promise an `onChange` handler
31+
* returns; the cropper does not await `onConfirm`), so a failure not caught
32+
* there is reported nowhere — it escapes as an unhandled rejection.
33+
*/
34+
function uploadFailedMessage(t: TranslateFn, name: string, err: unknown): string {
35+
return t('fields.file.uploadFailed', {
36+
defaultValue: `Failed to upload "${name}": ${(err as Error).message}`,
37+
name,
38+
error: (err as Error).message,
39+
});
40+
}
41+
2642
/**
2743
* ImageField - Image upload widget with preview thumbnails
2844
* Supports single and multiple image uploads with drag-and-drop and preview display
@@ -47,7 +63,10 @@ export function ImageField({ value, onChange, field, readonly, onUploadingChange
4763
const { upload } = useUpload();
4864
const { t } = useObjectTranslation();
4965
const [uploading, setUploading] = useState(false);
50-
/** Client-side rejections (oversize picks), cleared on the next attempt. */
66+
/**
67+
* Per-pick failures shown under the button, cleared on the next attempt:
68+
* client-side rejections (oversize picks) and failed uploads.
69+
*/
5170
const [errors, setErrors] = useState<string[]>([]);
5271
// Display details of just-uploaded images, keyed by their new `sys_file` id.
5372
// Submitting the reference form means the field value no longer carries the
@@ -97,6 +116,10 @@ export function ImageField({ value, onChange, field, readonly, onUploadingChange
97116
} else {
98117
onChange(next);
99118
}
119+
} catch (err) {
120+
// The original image stays in place; the dialog closes in `finally`,
121+
// so the message lands in the field's error row, not behind it.
122+
setErrors([uploadFailedMessage(t as TranslateFn, name, err)]);
100123
} finally {
101124
releaseScope();
102125
setUploading(false);
@@ -176,19 +199,30 @@ export function ImageField({ value, onChange, field, readonly, onUploadingChange
176199
});
177200
setErrors(rejections);
178201
if (validFiles.length === 0) return;
202+
const failures: string[] = [];
179203

180204
// Released only after `onChange` has handed the value over, and whether or
181205
// not this widget is still mounted by then — see FileField's pipeline.
182206
const releaseScope = holdScope();
183207
setUploading(true);
184208
try {
185-
const imageObjects = await Promise.all(
209+
// Each pick is caught on its own, as FileField does: a failed pick is
210+
// reported and not added, and the picks that did upload still land.
211+
const uploaded = await Promise.all(
186212
validFiles.map(async (file) => {
187-
const result = await upload(file);
188-
remember(result, file.name);
189-
return fileValueForSubmit(result, file.name);
213+
try {
214+
const result = await upload(file);
215+
remember(result, file.name);
216+
return fileValueForSubmit(result, file.name);
217+
} catch (err) {
218+
failures.push(uploadFailedMessage(t as TranslateFn, file.name, err));
219+
return null;
220+
}
190221
}),
191222
);
223+
if (failures.length > 0) setErrors([...rejections, ...failures]);
224+
const imageObjects = uploaded.filter((v) => v !== null);
225+
if (imageObjects.length === 0) return;
192226

193227
if (multiple) {
194228
onChange([...images, ...imageObjects]);
@@ -287,9 +321,8 @@ export function ImageField({ value, onChange, field, readonly, onUploadingChange
287321
: t('fields.image.upload')}
288322
</Button>
289323

290-
{/* Client-side rejections (oversize picks). Same presentation as
291-
FileField's error row — this widget previously had no surface for
292-
them at all, because it never rejected anything (objectui#4141). */}
324+
{/* Oversize picks (objectui#4141) and failed uploads (objectui#10226).
325+
Same presentation as FileField's error row. */}
293326
{errors.length > 0 && (
294327
<div className="space-y-0.5">
295328
{errors.map((err, i) => (
Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
/**
2+
* ImageField — a FAILED upload is reported inline, the image is not added,
3+
* and the rejection never escapes the handler (objectui#10226).
4+
*
5+
* The defect this pins: both upload paths (`handleFileChange` for the native
6+
* picker, `handleCropConfirm` for the crop dialog) were `try … finally` with no
7+
* `catch`. Both are invoked fire-and-forget — React ignores the promise an
8+
* `onChange` handler returns, and the cropper calls `onConfirm` without
9+
* awaiting it — so a rejecting upload surfaced as an unhandled promise
10+
* rejection and the user saw nothing at all. FileField, on the same failure
11+
* through the same `useUpload()` transport, renders `fields.file.uploadFailed`.
12+
*
13+
* So each failure case asserts three facts together: the translated failure
14+
* text is in the DOM, `onChange` was not called with the failed image, and the
15+
* process saw zero `unhandledRejection` events. The success cases are the
16+
* control — the same harness with a resolving transport still adds the image.
17+
*/
18+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
19+
import { render, screen, fireEvent, waitFor } from '@testing-library/react';
20+
import { UploadProvider, type UploadAdapter } from '@object-ui/providers';
21+
import { ImageField } from './ImageField';
22+
// Imported at module scope (not awaited in a hook) so the widget's
23+
// `React.lazy(() => import('./ImageCropperDialog'))` resolves from the ESM
24+
// cache instead of racing RTL's 1000ms `findBy` budget — AGENTS.md §测试纪律.
25+
import './ImageCropperDialog';
26+
27+
function image(name: string): File {
28+
return new File(['x'], name, { type: 'image/png' });
29+
}
30+
31+
/**
32+
* `upload` rejects for any name listed in `failing`, resolves otherwise. A crop
33+
* uploads a bare `Blob`, which carries no name, so it is keyed `cropped.png`.
34+
*/
35+
function renderField(field: Record<string, unknown>, value: unknown, failing: string[]) {
36+
const upload = vi.fn(async (f: File | Blob) => {
37+
const name = (f as File).name ?? 'cropped.png';
38+
if (failing.includes(name)) throw new Error('network down');
39+
return { url: `https://cdn.example/${name}`, name, size: f.size, mimeType: 'image/png' };
40+
});
41+
const adapter: UploadAdapter = { name: 'spy', upload };
42+
const onChange = vi.fn();
43+
render(
44+
<UploadProvider adapter={adapter}>
45+
<ImageField field={field as any} value={value} onChange={onChange} />
46+
</UploadProvider>,
47+
);
48+
return { upload, onChange, input: document.querySelector('input[type="file"]') as HTMLInputElement };
49+
}
50+
51+
/**
52+
* Node's `process`, reached through `globalThis` with only the two members this
53+
* file uses typed — this package's test program deliberately carries no
54+
* `@types/node` (see `tsconfig.test.json`).
55+
*/
56+
type RejectionListener = (reason: unknown) => void;
57+
const nodeProcess = (globalThis as unknown as {
58+
process: {
59+
on(event: 'unhandledRejection', listener: RejectionListener): void;
60+
off(event: 'unhandledRejection', listener: RejectionListener): void;
61+
};
62+
}).process;
63+
64+
/** Lets the event loop turn so a pending `unhandledRejection` is dispatched. */
65+
const settle = () => new Promise((resolve) => setTimeout(resolve, 20));
66+
67+
const existing = { name: 'cover.png', url: 'https://cdn.example/cover.png', mime_type: 'image/png' };
68+
69+
describe('ImageField — a failed upload is reported, not swallowed (#10226)', () => {
70+
let unhandled: unknown[];
71+
const onUnhandled = (reason: unknown) => {
72+
unhandled.push(reason);
73+
};
74+
beforeEach(() => {
75+
unhandled = [];
76+
nodeProcess.on('unhandledRejection', onUnhandled);
77+
});
78+
afterEach(() => {
79+
nodeProcess.off('unhandledRejection', onUnhandled);
80+
});
81+
82+
it('picker: shows the translated failure, adds nothing, leaks no rejection', async () => {
83+
const { upload, onChange, input } = renderField({ name: 'p_cover' }, undefined, ['bad.png']);
84+
85+
fireEvent.change(input, { target: { files: [image('bad.png')] } });
86+
87+
await waitFor(() => expect(upload).toHaveBeenCalledTimes(1));
88+
await settle();
89+
expect.soft(unhandled).toHaveLength(0);
90+
expect.soft(onChange).not.toHaveBeenCalled();
91+
await screen.findByText('Failed to upload "bad.png": network down');
92+
// The button leaves its uploading state, so the user can retry.
93+
expect(screen.getByTestId('image-field-upload-button').hasAttribute('disabled')).toBe(false);
94+
});
95+
96+
it('picker (multiple): adds the successful picks and reports only the failed one', async () => {
97+
const { upload, onChange, input } = renderField({ name: 'p_gallery', multiple: true }, undefined, ['bad.png']);
98+
99+
fireEvent.change(input, { target: { files: [image('good.png'), image('bad.png')] } });
100+
101+
await waitFor(() => expect(upload).toHaveBeenCalledTimes(2));
102+
await settle();
103+
expect.soft(unhandled).toHaveLength(0);
104+
await screen.findByText('Failed to upload "bad.png": network down');
105+
await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1));
106+
const added = onChange.mock.calls[0][0] as unknown[];
107+
expect(added).toHaveLength(1);
108+
expect(JSON.stringify(added)).toContain('https://cdn.example/good.png');
109+
expect(JSON.stringify(added)).not.toContain('bad.png');
110+
});
111+
112+
it('picker control: a resolving upload is still added', async () => {
113+
const { onChange, input } = renderField({ name: 'p_cover' }, undefined, []);
114+
115+
fireEvent.change(input, { target: { files: [image('good.png')] } });
116+
117+
await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1));
118+
await settle();
119+
expect(JSON.stringify(onChange.mock.calls[0][0])).toContain('https://cdn.example/good.png');
120+
expect(screen.queryByText(/Failed to upload/)).toBeNull();
121+
expect(unhandled).toHaveLength(0);
122+
});
123+
124+
it('crop: shows the translated failure, keeps the original image, leaks no rejection', async () => {
125+
const { upload, onChange } = renderField({ name: 'p_cover', crop: true }, existing, ['cropped.png']);
126+
127+
fireEvent.click(screen.getByTestId('image-field-crop-0'));
128+
fireEvent.click(await screen.findByTestId('fake-crop-confirm'));
129+
130+
await waitFor(() => expect(upload).toHaveBeenCalledTimes(1));
131+
await settle();
132+
expect.soft(unhandled).toHaveLength(0);
133+
expect.soft(onChange).not.toHaveBeenCalled();
134+
await screen.findByText('Failed to upload "cover.png": network down');
135+
// The dialog closes so the message is not hidden behind it.
136+
expect(screen.queryByTestId('fake-crop-confirm')).toBeNull();
137+
});
138+
139+
it('crop control: a resolving upload still replaces the image', async () => {
140+
const { onChange } = renderField({ name: 'p_cover', crop: true }, existing, []);
141+
142+
fireEvent.click(screen.getByTestId('image-field-crop-0'));
143+
fireEvent.click(await screen.findByTestId('fake-crop-confirm'));
144+
145+
await waitFor(() => expect(onChange).toHaveBeenCalledTimes(1));
146+
await settle();
147+
expect(JSON.stringify(onChange.mock.calls[0][0])).toContain('https://cdn.example/cropped.png');
148+
expect(screen.queryByText(/Failed to upload/)).toBeNull();
149+
expect(unhandled).toHaveLength(0);
150+
});
151+
});
152+
153+
/**
154+
* The real cropper is a canvas + `toBlob` pipeline that happy-dom cannot run;
155+
* this stand-in drives the one contract ImageField has with it — `onConfirm`
156+
* hands back a blob and a name, fire-and-forget, exactly as the real dialog
157+
* calls it (it does not await the returned promise).
158+
*/
159+
vi.mock('./ImageCropperDialog', () => ({
160+
ImageCropperDialog: ({ onConfirm }: { onConfirm: (blob: Blob, name: string) => void }) => (
161+
<button
162+
type="button"
163+
data-testid="fake-crop-confirm"
164+
onClick={() => onConfirm(new Blob(['x'], { type: 'image/png' }), 'cover.png')}
165+
>
166+
confirm
167+
</button>
168+
),
169+
}));

0 commit comments

Comments
 (0)