Skip to content

Commit f646b60

Browse files
committed
fix(console): the attachments panel attaches through the upload's own completion
Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L9qibvynRbCtMndW8EHNGc
1 parent 6ff78ac commit f646b60

4 files changed

Lines changed: 157 additions & 50 deletions

File tree

‎ui/packages/app-shell/src/views/RecordAttachmentsPanel.tsx‎

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,11 @@ import { RECORD_PAGE_ATTACHMENT_ROWS, recordAttachmentsCountField } from './reco
4646
*
4747
* Storage model: one `sys_file` row per uploaded blob (three-step
4848
* presigned upload via @object-ui/data-objectstack's ObjectStack adapter), one
49-
* `sys_attachment` join row linking it to `(parent_object, parent_id)`.
49+
* `sys_attachment` join row linking it to `(parent_object, parent_id)`. The
50+
* join row is written by the upload's own completion (`attachTo`), as the
51+
* signed-in user: the server's attach verb, never a `sys_attachment` create
52+
* through the generic data door, which a member who may attach files but not
53+
* read `sys_file` is refused (`reference_not_found` on `file_id`).
5054
* Downloads fetch a short-lived signed URL from `/storage/files/:fileId/url`
5155
* with the console's Bearer token (the endpoint requires an authenticated
5256
* session for attachments-scope files, objectstack-ai/objectstack#2970), then open it.
@@ -65,9 +69,9 @@ interface AttachmentRow {
6569
export interface RecordAttachmentsPanelProps {
6670
objectName: string;
6771
recordId: string;
68-
/** ObjectUI DataSource (sys_attachment CRUD goes through the generic data path). */
72+
/** ObjectUI DataSource (the list read and the delete go through the generic data path; the attach does not). */
6973
dataSource: any;
70-
/** Current user id, stamped on `uploaded_by`. */
74+
/** Current user id. The server stamps `uploaded_by` from the session; the panel no longer sends it. */
7175
currentUserId?: string | null;
7276
className?: string;
7377
}
@@ -175,7 +179,6 @@ export const RecordAttachmentsPanel: React.FC<RecordAttachmentsPanelProps> = ({
175179
objectName,
176180
recordId,
177181
dataSource,
178-
currentUserId,
179182
className,
180183
}) => {
181184
const { t } = useObjectTranslation();
@@ -216,10 +219,10 @@ export const RecordAttachmentsPanel: React.FC<RecordAttachmentsPanelProps> = ({
216219
* and a delete go through the storage route, not `sys_attachment`'s generic
217220
* data door, so that door's operation set says nothing about them.
218221
*
219-
* Upload needs `create`. Without it the three-step presigned flow below
220-
* commits a `sys_file` and only the final `sys_attachment` insert is
221-
* refused: the user meets the error after the bytes are stored, and the file
222-
* stays committed with nothing attached to it.
222+
* Upload needs `create`. Without it the attach at the end of the upload
223+
* below is refused after the bytes are stored (the server leaves the file
224+
* pending, and its retention reclaims it); hiding Upload spares the user
225+
* that round trip.
223226
*
224227
* Delete needs `delete`, and being the uploader does not change that. The
225228
* server asks the object grant first (plugin-security's CRUD check wraps the
@@ -252,9 +255,17 @@ export const RecordAttachmentsPanel: React.FC<RecordAttachmentsPanelProps> = ({
252255
// upload adapter and the download-URL fetch — the storage routes require a
253256
// session and there is no cookie for `credentials: 'include'` to carry.
254257
const authFetch = React.useMemo(() => createAuthenticatedFetch(), []);
258+
// Every upload from this panel is attached to this record by the call that
259+
// completes it (`attachTo`): the server's attach verb, run as the user.
255260
const adapter = React.useMemo(
256-
() => createObjectStackUploadAdapter({ baseUrl, scope: 'attachments', fetchImpl: authFetch }),
257-
[baseUrl, authFetch],
261+
() =>
262+
createObjectStackUploadAdapter({
263+
baseUrl,
264+
scope: 'attachments',
265+
fetchImpl: authFetch,
266+
...(objectName && recordId ? { attachTo: { parentObject: objectName, parentId: recordId } } : {}),
267+
}),
268+
[baseUrl, authFetch, objectName, recordId],
258269
);
259270

260271
/** Map the server's fail-closed 40x codes (objectstack-ai/objectstack#2755, objectstack-ai/objectstack#2970) to friendly copy. */
@@ -390,27 +401,18 @@ export const RecordAttachmentsPanel: React.FC<RecordAttachmentsPanelProps> = ({
390401

391402
const handleFiles = React.useCallback(
392403
async (files: FileList | null) => {
393-
if (!files || files.length === 0 || !dataSource) return;
404+
if (!files || files.length === 0 || !dataSource || !objectName || !recordId) return;
394405
setUploading(true);
395406
setError(null);
396407
try {
397408
for (const file of Array.from(files)) {
398-
// 1) blob → sys_file via the canonical presigned three-step flow
409+
// blob → sys_file → sys_attachment: the presigned three-step flow,
410+
// whose completion attaches the file to this record as the user
411+
// (the server checks the create grant, edit access to the record,
412+
// `enable.files`, and that the file is the user's own upload).
399413
const uploaded = await adapter.upload(file);
400-
const fileId = (uploaded.meta as any)?.fileId as string | undefined;
401-
if (!fileId) throw new Error('Upload did not return a fileId');
402-
// 2) join row → sys_attachment (server enforces enable.files)
403-
await dataSource.create('sys_attachment', {
404-
parent_object: objectName,
405-
parent_id: recordId,
406-
file_id: fileId,
407-
file_name: uploaded.name ?? file.name,
408-
mime_type: uploaded.mimeType ?? file.type,
409-
size: uploaded.size ?? file.size,
410-
// Back-compat with pre-objectstack-ai/objectstack#2755 servers; a current server stamps
411-
// `uploaded_by` from the session and ignores this value.
412-
...(currentUserId ? { uploaded_by: currentUserId } : {}),
413-
});
414+
const attachmentId = (uploaded.meta as any)?.attachmentId as string | undefined;
415+
if (!attachmentId) throw new Error('The server completed the upload but did not attach it to this record');
414416
}
415417
await refresh();
416418
} catch (err: any) {
@@ -420,7 +422,7 @@ export const RecordAttachmentsPanel: React.FC<RecordAttachmentsPanelProps> = ({
420422
if (inputRef.current) inputRef.current.value = '';
421423
}
422424
},
423-
[adapter, dataSource, objectName, recordId, currentUserId, refresh],
425+
[adapter, dataSource, objectName, recordId, refresh],
424426
);
425427

426428
/**

‎ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.affordanceGrant-12047.test.tsx‎

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,17 @@ import { render, screen, waitFor, within, act } from '@testing-library/react';
3636
import userEvent from '@testing-library/user-event';
3737
import { MePermissionsProvider, type MePermissionsAuthenticatedResponse } from '@object-ui/permissions';
3838

39-
/** The fake presigned adapter: `upload` resolving means a `sys_file` was committed. */
39+
/** The fake presigned adapter: `upload` resolving means the file was committed and attached. */
4040
const adapterUpload = vi.fn();
41+
/** The options the panel built its adapter with — `attachTo` names the record. */
42+
const adapterOptions = vi.fn();
4143

4244
vi.mock('@object-ui/data-objectstack', async (importOriginal) => ({
4345
...(await importOriginal<Record<string, unknown>>()),
44-
createObjectStackUploadAdapter: () => ({ upload: adapterUpload }),
46+
createObjectStackUploadAdapter: (opts: unknown) => {
47+
adapterOptions(opts);
48+
return { upload: adapterUpload };
49+
},
4550
}));
4651

4752
import { RecordAttachmentsPanel } from '../RecordAttachmentsPanel';
@@ -174,20 +179,15 @@ describe('RecordAttachmentsPanel — write affordances follow the sys_attachment
174179
});
175180

176181
it('CONTROL — with Upload shown and the server refusing the attach, the refusal message still appears', async () => {
177-
adapterUpload.mockResolvedValue({
178-
name: 'quote.pdf',
179-
mimeType: 'application/pdf',
180-
size: 10,
181-
meta: { fileId: 'f-new' },
182-
});
183-
const dataSource = makeDataSource({
184-
create: vi.fn(async () => {
185-
throw Object.assign(new Error("[Security] Access denied: operation 'insert' on sys_attachment"), {
186-
httpStatus: 403,
187-
code: 'PERMISSION_DENIED',
188-
});
182+
// The attach is the upload's own completion (`attachTo`), so the server's
183+
// refusal is the upload's.
184+
adapterUpload.mockRejectedValue(
185+
Object.assign(new Error('Upload completion failed (403): {"success":false,"error":{"code":"PERMISSION_DENIED"}}'), {
186+
status: 403,
187+
code: 'PERMISSION_DENIED',
189188
}),
190-
});
189+
);
190+
const dataSource = makeDataSource();
191191
const { container } = renderWithGrant({ allowCreate: true }, dataSource);
192192
await rowsLoaded();
193193
expect(screen.getByRole('button', { name: 'Upload' })).toBeInTheDocument();
@@ -200,10 +200,11 @@ describe('RecordAttachmentsPanel — write affordances follow the sys_attachment
200200
expect(screen.getByRole('alert')).toHaveTextContent("You don't have permission to do that."),
201201
);
202202
expect(adapterUpload).toHaveBeenCalledTimes(1);
203-
expect(dataSource.create).toHaveBeenCalledWith(
204-
'sys_attachment',
205-
expect.objectContaining({ parent_object: 'att_case', parent_id: 'r1', file_id: 'f-new' }),
203+
expect(adapterOptions).toHaveBeenLastCalledWith(
204+
expect.objectContaining({ scope: 'attachments', attachTo: { parentObject: 'att_case', parentId: 'r1' } }),
206205
);
206+
// No `sys_attachment` create through the generic data door.
207+
expect(dataSource.create).not.toHaveBeenCalled();
207208
});
208209

209210
it('no flash while permissions load: a read-only answer never shows Upload, not even before it arrives', async () => {

‎ui/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsx‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -846,3 +846,81 @@ describe('RecordAttachmentsPanel — mobile shell openFile', () => {
846846
expect(host.openFile).not.toHaveBeenCalled();
847847
});
848848
});
849+
850+
/**
851+
* The upload attaches through the server's attach verb: the presigned upload's
852+
* own completion carries `attachTo` (`POST /api/v1/storage/upload/complete`),
853+
* and the server commits the file and writes its `sys_attachment` row as the
854+
* signed-in user. The panel never creates `sys_attachment` through the generic
855+
* data door, which refuses a member who may attach but cannot read `sys_file`.
856+
* Driven through the REAL ObjectStack upload adapter over a stubbed `fetch`.
857+
*/
858+
describe('RecordAttachmentsPanel — upload attaches through the upload’s own completion', () => {
859+
const ok = (data: unknown) =>
860+
new Response(JSON.stringify({ success: true, data }), { status: 200, headers: { 'content-type': 'application/json' } });
861+
862+
function stubStorage(complete: () => Response) {
863+
const calls: Array<{ url: string; method: string; body: unknown }> = [];
864+
const spy = vi.spyOn(globalThis, 'fetch').mockImplementation(async (input: RequestInfo | URL, init?: RequestInit) => {
865+
const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url;
866+
const method = init?.method ?? 'GET';
867+
const body = typeof init?.body === 'string' ? JSON.parse(init.body) : init?.body;
868+
calls.push({ url, method, body });
869+
if (url.endsWith('/api/v1/storage/upload/presigned')) {
870+
return ok({ uploadUrl: '/api/v1/storage/_local/raw/tok', method: 'PUT', fileId: 'f-new', headers: {} });
871+
}
872+
if (url.endsWith('/api/v1/storage/_local/raw/tok')) return ok({ key: 'attachments/f-new.pdf' });
873+
if (url.endsWith('/api/v1/storage/upload/complete')) return complete();
874+
throw new Error(`unexpected fetch ${method} ${url}`);
875+
});
876+
return { spy, calls };
877+
}
878+
879+
async function uploadOne() {
880+
const dataSource = makeDataSource();
881+
const { container } = render(
882+
<RecordAttachmentsPanel objectName="att_case" recordId="r1" dataSource={dataSource} currentUserId="me" />,
883+
);
884+
await waitFor(() => expect(screen.getByText('report.pdf')).toBeInTheDocument());
885+
const input = container.querySelector('input[type="file"]') as HTMLInputElement;
886+
await userEvent.setup().upload(input, new File(['%PDF'], 'quote.pdf', { type: 'application/pdf' }));
887+
return dataSource;
888+
}
889+
890+
afterEach(() => vi.restoreAllMocks());
891+
892+
it('completes the upload with attachTo naming this record, and creates no sys_attachment through the data door', async () => {
893+
const { calls } = stubStorage(() => ok({ fileId: 'f-new', name: 'quote.pdf', attachmentId: 'a-new' }));
894+
const dataSource = await uploadOne();
895+
await waitFor(() => expect(dataSource.find).toHaveBeenCalledTimes(2)); // the list re-read after the attach
896+
const complete = calls.find((c) => c.url.endsWith('/upload/complete'));
897+
expect(complete).toMatchObject({
898+
method: 'POST',
899+
body: { fileId: 'f-new', attachTo: { parentObject: 'att_case', parentId: 'r1' } },
900+
});
901+
expect(calls.find((c) => c.url.endsWith('/upload/presigned'))?.body).toMatchObject({ scope: 'attachments' });
902+
expect(dataSource.create).not.toHaveBeenCalled();
903+
expect(screen.queryByRole('alert')).not.toBeInTheDocument();
904+
});
905+
906+
it('maps the attach verb’s ATTACHMENT_PARENT_ACCESS refusal to the friendly copy', async () => {
907+
stubStorage(
908+
() =>
909+
new Response(
910+
JSON.stringify({ success: false, error: { code: 'ATTACHMENT_PARENT_ACCESS', message: 'Cannot attach to att_case/r1' } }),
911+
{ status: 403, headers: { 'content-type': 'application/json' } },
912+
),
913+
);
914+
const dataSource = await uploadOne();
915+
await waitFor(() =>
916+
expect(screen.getByRole('alert')).toHaveTextContent("You don't have access to attach files to this record."),
917+
);
918+
expect(dataSource.create).not.toHaveBeenCalled();
919+
});
920+
921+
it('a completion that attached nothing is an error, never a silent success', async () => {
922+
stubStorage(() => ok({ fileId: 'f-new', name: 'quote.pdf' }));
923+
await uploadOne();
924+
await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent('did not attach it to this record'));
925+
});
926+
});

‎ui/packages/data-objectstack/src/upload-adapter.ts‎

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,18 @@ export interface ObjectStackUploadAdapterOptions {
4545
* declares (objectui#12055).
4646
*/
4747
scope?: API.UploadScope;
48+
/**
49+
* Attach every upload from this adapter to this record, in the call that
50+
* completes it: the completion request carries `attachTo`, and the server
51+
* commits the file and writes its `sys_attachment` row as one unit, as the
52+
* signed-in caller (the record attachments surface's attach verb). The
53+
* result's `meta.attachmentId` names the new attachment. The server attaches
54+
* only the caller's own pending upload in the `attachments` scope, so set
55+
* `scope: 'attachments'` with it. A refused attach fails the upload with the
56+
* server's refusal and leaves the file pending, never committed and
57+
* unattached.
58+
*/
59+
attachTo?: { parentObject: string; parentId: string };
4860
/** Override `fetch` (tests, ServiceWorker queues). */
4961
fetchImpl?: typeof fetch;
5062
/**
@@ -64,7 +76,8 @@ export interface ObjectStackUploadAdapterOptions {
6476
* 1. POST `/upload/presigned` → `{ uploadUrl, fileId, downloadUrl }`
6577
* 2. PUT `uploadUrl` → uploads the raw bytes (HMAC token)
6678
* 3. POST `/upload/complete` → marks the file ready and registers
67-
* the `sys_file` record
79+
* the `sys_file` record — and, with
80+
* `attachTo`, attaches it to that record
6881
*
6982
* Falls back to a canonical (non-signed) download URL so the returned
7083
* `UploadResult.url` is stable across signed-URL expiry — components
@@ -136,14 +149,27 @@ export function createObjectStackUploadAdapter(
136149
credentials,
137150
headers: { 'Content-Type': 'application/json' },
138151
signal: options.signal,
139-
body: JSON.stringify({ fileId }),
152+
body: JSON.stringify(opts.attachTo ? { fileId, attachTo: opts.attachTo } : { fileId }),
140153
});
141154
if (!completeRes.ok) {
142155
const text = await completeRes.text().catch(() => '');
143-
throw new Error(
144-
`Upload completion failed (${completeRes.status}): ${text || completeRes.statusText}`,
156+
// The server's refusal code and status ride on the error, so a caller
157+
// maps a refused attach (`ATTACHMENT_PARENT_ACCESS`, `FILES_DISABLED`,
158+
// `PERMISSION_DENIED`…) without parsing the message.
159+
let code: string | undefined;
160+
try {
161+
const parsed = JSON.parse(text);
162+
if (typeof parsed?.error?.code === 'string') code = parsed.error.code;
163+
} catch {
164+
// Not the declared envelope — the message keeps the raw text.
165+
}
166+
throw Object.assign(
167+
new Error(`Upload completion failed (${completeRes.status}): ${text || completeRes.statusText}`),
168+
{ status: completeRes.status, ...(code ? { code } : {}) },
145169
);
146170
}
171+
const completeBody = opts.attachTo ? await completeRes.json().catch(() => null) : null;
172+
const attachmentId = (completeBody?.data ?? completeBody)?.attachmentId as string | undefined;
147173

148174
// Use the canonical (stable) download endpoint as the field value
149175
// so a saved record keeps working past signed-URL expiry. This
@@ -156,7 +182,7 @@ export function createObjectStackUploadAdapter(
156182
name,
157183
size: file.size,
158184
mimeType,
159-
meta: { fileId, scope: opts.scope },
185+
meta: { fileId, scope: opts.scope, ...(attachmentId ? { attachmentId } : {}) },
160186
};
161187
},
162188
};

0 commit comments

Comments
 (0)