Files
multica/packages/core/hooks/use-file-upload.ts
Multica Eve 57d1a0a00f fix(quick-create): track concurrent uploads with in-flight counter (MUL-3339) (#4562)
`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
    `![alt](blob:...)` 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>
2026-06-25 14:11:59 +08:00

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 };
}