mirror of
https://github.com/multica-ai/multica.git
synced 2026-07-26 12:35:35 +02:00
`useFileUpload` exposed a single `uploading: boolean` shared across all
concurrent upload calls. When the user drag-dropped N images into the
Quick Create modal, the first upload's `finally` flipped the flag back
to false while N-1 uploads were still in flight. The submit gate (which
only checked `uploading`) re-enabled, and on submit:
- `getMarkdown()` ran `stripBlobUrls()` and erased every still-pending
`` placeholder from the prompt.
- `pendingAttachments` only contained the first-completed upload, so
`activeAttachmentIds` shipped a single ID.
- The remaining attachment rows never got linked to the new issue —
their `issue_id` stayed NULL and the UI showed "Attachment doesn't
exist".
Fix:
1. Replace the boolean with an in-flight counter in `useFileUpload`.
`uploading = inFlight > 0` so the flag stays true until ALL concurrent
uploads resolve (or reject — the `finally` decrements either way).
The public return shape is unchanged; every existing call site keeps
working.
2. Belt-and-suspenders: add `editorRef.current?.hasActiveUploads()` to
the quick-create submit gate. The editor already tracks per-node
`uploading` attrs (the source of truth for "is there a blob preview
still resolving"); checking it on submit guarantees the strip step
can never run against an in-flight image even if some future code
path mis-clears `uploading`.
3. Regression coverage in `use-file-upload.test.ts`:
- Fire two concurrent uploads, resolve them out of order, assert
`uploading` stays true until BOTH resolve. Confirmed to fail
against the pre-fix code.
- Reject one of the concurrent uploads, assert the counter still
decrements correctly (the `finally` runs on rejection).
The mock ContentEditor in `quick-create-issue.test.tsx` gains a
`hasActiveUploads: () => false` stub so the new defense call doesn't
explode in existing tests.
Verified: `pnpm test` on @multica/core (663 tests) and @multica/views
(1471 tests) both green; typecheck + lint clean on both packages.
Co-authored-by: Eve <eve@multica-ai.local>
Co-authored-by: multica-agent <github@multica.ai>
137 lines
6.0 KiB
TypeScript
137 lines
6.0 KiB
TypeScript
"use client";
|
|
|
|
import { useState, useCallback } from "react";
|
|
import type { ApiClient } from "../api/client";
|
|
import type { Attachment } from "../types";
|
|
import { attachmentDownloadPath } from "../types/attachment-url";
|
|
import { MAX_FILE_SIZE } from "../constants/upload";
|
|
|
|
// Carries the full Attachment so editors that need preview metadata
|
|
// (`content_type`, `download_url`) get it directly. Two URL fields are
|
|
// surfaced because they have different lifetimes:
|
|
//
|
|
// `link` — the same value as `att.url`. Short-lived for the
|
|
// LocalStorage backend (HMAC-signed `/uploads/<key>`)
|
|
// and a long-lived CDN URL on S3 / CloudFront. This
|
|
// is what avatar / logo callers persist into
|
|
// `avatar_url` style fields, and what URL-only
|
|
// consumers (Markdown renderers without a record
|
|
// in hand) get to load directly. Keeping it
|
|
// semantically equal to `att.url` preserves the
|
|
// pre-MUL-3130 contract for non-markdown callers
|
|
// so avatar uploads do not get rerouted through
|
|
// the workspace-membership-gated download endpoint.
|
|
//
|
|
// `markdownLink` — the URL the editor writes into markdown bodies.
|
|
// Source: `att.markdown_url` from the server, which
|
|
// `buildMarkdownURL` picks per deployment policy
|
|
// (public CDN durable URL, or
|
|
// `<MULTICA_PUBLIC_URL>/api/attachments/<id>/download`).
|
|
// The contract is "absolute, no TTL, loads natively
|
|
// on every client" — that's what fixes the Desktop /
|
|
// mobile-webview regression where a site-relative
|
|
// `/api/attachments/<id>/download` couldn't resolve
|
|
// against `file://` (MUL-3192).
|
|
//
|
|
// Falls back through two layers when the server
|
|
// didn't populate `markdown_url`:
|
|
// 1. `attachmentDownloadPath(att.id)` — the legacy
|
|
// site-relative shape. Works on web (Next
|
|
// rewrite proxies /api/* to the API host) and
|
|
// is what older comments persist; render
|
|
// surfaces handle the absolutize for non-web
|
|
// clients via attachment.tsx's legacy compat.
|
|
// 2. `att.url` — the no-workspace avatar branch
|
|
// where there's no attachment-row id at all.
|
|
//
|
|
// MUL-3130 introduced the persisted-image regression by collapsing
|
|
// these two semantics into a single `link` field; MUL-3192 followed up
|
|
// by moving the durable-URL choice from the client to the server so
|
|
// the persisted shape is correct for the deployment without the client
|
|
// having to know whether it's running on web / desktop / mobile.
|
|
export type UploadResult = Attachment & {
|
|
link: string;
|
|
markdownLink: string;
|
|
};
|
|
|
|
export interface UploadContext {
|
|
issueId?: string;
|
|
commentId?: string;
|
|
chatSessionId?: string;
|
|
}
|
|
|
|
// pickMarkdownLink chooses the URL the editor will write into markdown.
|
|
//
|
|
// Order:
|
|
// 1. `att.markdown_url` — server-provided durable URL. This is the
|
|
// modern contract introduced in MUL-3192; the server (`buildMarkdownURL`)
|
|
// decides whether to emit a public CDN URL or an absolute API
|
|
// endpoint pinned to `MULTICA_PUBLIC_URL` based on the deployment.
|
|
// 2. `attachmentDownloadPath(att.id)` — site-relative legacy shape,
|
|
// retained for compatibility with backends old enough to predate
|
|
// MUL-3192. Web's Next rewrite makes this load; desktop / mobile
|
|
// surfaces hit the attachment.tsx legacy-absolutize fallback.
|
|
// 3. `att.url` — no attachment-row id (the no-workspace avatar branch
|
|
// of UploadFile). Markdown callers fall back to whatever storage
|
|
// backend produced for the upload; persistence is on the caller.
|
|
function pickMarkdownLink(att: Attachment): string {
|
|
if (att.markdown_url) return att.markdown_url;
|
|
if (att.id) return attachmentDownloadPath(att.id);
|
|
return att.url;
|
|
}
|
|
|
|
export function useFileUpload(
|
|
api: ApiClient,
|
|
onError?: (error: Error) => void,
|
|
) {
|
|
// In-flight counter, NOT a single boolean. Callers fire multiple uploads
|
|
// concurrently (drag-drop of N files, paste with multiple images) and the
|
|
// boolean shape would flip false as soon as the FIRST upload's finally ran
|
|
// — even though N-1 are still mid-request. Surfaces consuming `uploading`
|
|
// (the quick-create submit gate, the editor's "Uploading…" button label)
|
|
// would then unblock submit while uploads are still in flight, causing
|
|
// `stripBlobUrls` to erase the still-pending images from the markdown and
|
|
// their attachment ids never to be bound (MUL-3339).
|
|
//
|
|
// The exposed `uploading: boolean` keeps the existing call-site contract
|
|
// (`{ uploading } = useFileUpload(api)` everywhere); only the internal
|
|
// tracking shape changes.
|
|
const [inFlight, setInFlight] = useState(0);
|
|
const uploading = inFlight > 0;
|
|
|
|
const upload = useCallback(
|
|
async (file: File, ctx?: UploadContext): Promise<UploadResult | null> => {
|
|
if (file.size > MAX_FILE_SIZE) {
|
|
throw new Error("File exceeds 100 MB limit");
|
|
}
|
|
|
|
setInFlight((n) => n + 1);
|
|
try {
|
|
const att: Attachment = await api.uploadFile(file, {
|
|
issueId: ctx?.issueId,
|
|
commentId: ctx?.commentId,
|
|
chatSessionId: ctx?.chatSessionId,
|
|
});
|
|
return { ...att, link: att.url, markdownLink: pickMarkdownLink(att) };
|
|
} finally {
|
|
setInFlight((n) => n - 1);
|
|
}
|
|
},
|
|
[api],
|
|
);
|
|
|
|
const uploadWithToast = useCallback(
|
|
async (file: File, ctx?: UploadContext): Promise<UploadResult | null> => {
|
|
try {
|
|
return await upload(file, ctx);
|
|
} catch (err) {
|
|
onError?.(err instanceof Error ? err : new Error("Upload failed"));
|
|
return null;
|
|
}
|
|
},
|
|
[upload, onError],
|
|
);
|
|
|
|
return { upload, uploadWithToast, uploading };
|
|
}
|