Skip to content

Fix attachment lifecycle completion and send gating #713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
witbrock merged 1 commit into from
Sep 15, 2026
Merged

Fix attachment lifecycle completion and send gating #713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 32 additions & 56 deletions src/frontend/web/von_interface/static/js/chatTab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { CONVERSATION_LAYOUT_KEY, CONVERSATION_LAYOUT_KEYS, CONVERSATION_NARROW_
import { createVoiceConversation } from './voiceConversation.js';
import { getClientContext } from './clientContext.js';
import { createDictationController } from './dictation.js';
import { isConversationImageFile, initialiseImagePicker, renderImageAttachments, renderImageComposer, uploadConversationImage, imagesBlocked, imageItems, takeImages, restoreImages, descriptorsForIds } from "./utils/conversationImages.js";
import { isConversationImageFile, initialiseImagePicker, renderImageAttachments, renderImageComposer, uploadConversationImage, attachmentBlockMessage, imagesBlocked, imageItems, takeImages, restoreImages, descriptorsForIds } from "./utils/conversationImages.js";
// Chat Tab Module
import { annotateTurn, fetchWithTimeout, getJsonDetailed, getUserContext, getWindowSessionId, postJson, WINDOW_SESSION_HEADER } from './apiService.js';
import {
Expand Down Expand Up @@ -6802,9 +6802,9 @@ function updateSendButtonForCurrentChatState() {
if (queueSubmissionInFlight) {
sendButton.title = 'Saving this prompt to the conversation queue.';
} else if (imagePreparationBlocked && !uploadInFlight) {
sendButton.title = 'Remove the failed image attachment before sending.';
sendButton.title = attachmentBlockMessage(activeChatSessionId);
} else if (uploadInFlight) {
sendButton.title = ATTACHMENT_UPLOAD_SEND_BLOCK_MESSAGE;
sendButton.title = attachmentBlockMessage(activeChatSessionId);
} else {
sendButton.title = sessionBusy
? 'Queue a separate request after the current turn finishes. Use Steer to guide the active turn.'
Expand Down Expand Up @@ -21492,17 +21492,15 @@ let uploadUiState = {
};

const PENDING_FILE_COPY_SESSION_FALLBACK_KEY = '__pending_chat_session__';
const ATTACHMENT_UPLOAD_SEND_BLOCK_MESSAGE = (
'Wait for the attachment upload to finish before sending.'
);
const pendingFileCopyConceptIdsBySession = new Map();
const pendingFileCopyDisplayNamesBySession = new Map();
const uploadStatesBySession = new Map();

function getInFlightUploadStateForSession(sessionId = activeChatSessionId) {
const sessionKey = getPendingFileCopySessionKey(sessionId);
const uploadState = uploadStatesBySession.get(sessionKey) || null;
return uploadState?.inFlight === true ? uploadState : null;
// Once registered, individual attachments own readiness, not the batch promise.
return uploadState?.inFlight === true && !uploadState.itemsRegistered ? uploadState : null;
}

function normaliseTrustedUploadedFileCopyConceptId(value) {
Expand Down Expand Up @@ -21569,7 +21567,7 @@ function refreshConversationImages() {
syncConversationAttachmentWorkflowBinding(activeChatSessionId);
renderPendingAttachmentState();
});
updateSendButtonForCurrentChatState();
renderActiveUploadUi();
}

function syncConversationAttachmentWorkflowBinding(sessionId) {
Expand Down Expand Up @@ -21611,24 +21609,6 @@ function renderPendingAttachmentState() {
refreshAttachmentStatusRegionVisibility();
}

function rememberPendingUploadedFileCopyConceptId(
conceptId,
sessionId = activeChatSessionId,
displayName = null
) {
const normalisedConceptId = normaliseTrustedUploadedFileCopyConceptId(conceptId);
if (!normalisedConceptId) return;
const targetKey = getPendingFileCopySessionKey(sessionId);
pendingFileCopyConceptIdsBySession.set(targetKey, normalisedConceptId);
const normalisedDisplayName = normalisePendingAttachmentDisplayName(displayName);
if (normalisedDisplayName) {
pendingFileCopyDisplayNamesBySession.set(targetKey, normalisedDisplayName);
} else {
pendingFileCopyDisplayNamesBySession.delete(targetKey);
}
renderPendingAttachmentState();
}

function takePendingUploadedFileCopyBinding(sessionId) {
const targetKey = getPendingFileCopySessionKey(sessionId);
const targetConceptId = pendingFileCopyConceptIdsBySession.get(targetKey) || null;
Expand Down Expand Up @@ -21711,11 +21691,18 @@ function renderUploadStatus(message, type = 'info') {
function renderActiveUploadUi() {
const activeSessionKey = getPendingFileCopySessionKey(activeChatSessionId);
const activeUploadState = uploadStatesBySession.get(activeSessionKey) || null;
renderUploadStatus(
activeUploadState?.message || '',
activeUploadState?.tone || 'info'
);
setUploadButtonBusy(activeUploadState?.inFlight === true);
const items = imageItems(activeChatSessionId);
const unfinished = items.filter(item => item.state !== 'ready');
const preparing = !!getInFlightUploadStateForSession(activeChatSessionId);
const uploading = unfinished.some(item => ['queued', 'uploading', 'retrying'].includes(item.state));
const failed = unfinished.some(item => item.state === 'failed');
const message = preparing ? activeUploadState.message
: failed ? attachmentBlockMessage(activeChatSessionId)
: uploading ? `Uploading: ${unfinished.map(item => item.name).join(', ')}`
: items.length ? `Upload complete: ${items.length} ready.`
: activeUploadState?.tone === 'error' ? activeUploadState.message : '';
renderUploadStatus(message, failed ? 'error' : (preparing || uploading) ? 'uploading' : 'success');
setUploadButtonBusy(preparing || uploading);
updateSendButtonForCurrentChatState();
}

Expand Down Expand Up @@ -21930,29 +21917,14 @@ async function performUploadFilesToVon(list, uploadState) {
rekeyUploadStateForSession(uploadState, uploadTargetSessionId);
}

let successCount = 0;
let failureCount = 0;

for (const file of list) {
const uploaded = await uploadConversationImage(file, uploadTargetSessionId, buildChatFetchHeaders(), () => {
syncConversationAttachmentWorkflowBinding(uploadTargetSessionId);
renderPendingAttachmentState();
});
if (uploaded) {
successCount += 1;
if (!isConversationImageFile(file)) {
const item = imageItems(uploadTargetSessionId).findLast(item => item.name === file.name && item.descriptor);
if (item) rememberPendingUploadedFileCopyConceptId(item.descriptor.concept_id, uploadTargetSessionId, file.name);
}
} else failureCount += 1;
}

const summary = `Upload complete: ${successCount} succeeded${failureCount ? `, ${failureCount} failed` : ''}.`;
setUploadStatusForState(
uploadState,
summary,
failureCount ? 'error' : 'success'
);
// Register all files before yielding so unstarted members cannot escape the gate.
const uploads = list.map(file => uploadConversationImage(file, uploadTargetSessionId, buildChatFetchHeaders(), () => {
syncConversationAttachmentWorkflowBinding(uploadTargetSessionId);
renderPendingAttachmentState();
}));
uploadState.itemsRegistered = true;
renderActiveUploadUi();
await Promise.all(uploads);
clearUploadStatusAfterDelay(uploadState);
}

Expand All @@ -21973,10 +21945,14 @@ function uploadFilesToVon(files) {
const sessionKey = getPendingFileCopySessionKey(targetSessionId);
const selectionFingerprint = buildUploadSelectionFingerprint(list);
const existingState = uploadStatesBySession.get(sessionKey) || null;
if (existingState?.inFlight && existingState.promise) {
if (existingState?.promise && (getInFlightUploadStateForSession(targetSessionId)
|| imageItems(targetSessionId).some(item => ['queued', 'uploading', 'retrying'].includes(item.state)))) {
const repeatedSelection = (
existingState.selectionFingerprint === selectionFingerprint
);
if (!repeatedSelection) {
showToast('Another upload is already in progress; this additional selection was not uploaded.', 'info');
}
setUploadStatusForState(
existingState,
repeatedSelection
Expand Down Expand Up @@ -37317,7 +37293,7 @@ async function handleSendPrompt(options = {}) {
directComposerSubmission
&& (getInFlightUploadStateForSession(targetSessionId) || imagesBlocked(targetSessionId))
) {
showToast(ATTACHMENT_UPLOAD_SEND_BLOCK_MESSAGE, 'info');
showToast(attachmentBlockMessage(targetSessionId), 'info');
updateSendButtonForCurrentChatState();
return;
}
Expand Down
37 changes: 27 additions & 10 deletions src/frontend/web/von_interface/static/js/utils/conversationImages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,16 @@
// Durable attachment descriptors (historical image API); originals remain behind authenticated routes.
// Draft lifecycle: queued -> uploading -> ready | failed; failed -> retrying
// -> ready | failed. Removal is terminal and late transport results are ignored.
// Descriptors are payload, while state alone owns rendering and send readiness.
const pending = new Map();
const known = new Map();
export function imageItems(sessionId) { return pending.get(sessionId) || []; }
export function imagesBlocked(sessionId) { return imageItems(sessionId).some(x => !x.descriptor); }
export function imagesBlocked(sessionId) { return imageItems(sessionId).some(x => x.state !== 'ready'); }
export function attachmentBlockMessage(sessionId) {
return imageItems(sessionId).some(x => x.state === 'failed')
? 'Attachment upload failed. Retry or remove the failed attachment before sending.'
: 'Wait for the attachment upload to finish before sending.';
}
export function renderImageAttachments(parent, images) {
if (!parent || !Array.isArray(images) || !images.length) return;
const gallery = document.createElement('div');
Expand Down Expand Up @@ -68,7 +76,8 @@ export function renderImageComposer(parent, sessionId, changed) {
for (const item of imageItems(sessionId)) {
const card = document.createElement('div');
card.className = 'conversation-image-draft';
if (item.descriptor) {
card.dataset.attachmentState = item.state;
if (item.state === 'ready') {
renderImageAttachments(card, [item.descriptor]);
card.querySelectorAll('img').forEach(img => { img.style.maxHeight = '100px'; });
}
Expand All @@ -80,7 +89,7 @@ export function renderImageComposer(parent, sessionId, changed) {
card.appendChild(preview);
}
const label = document.createElement('span');
label.textContent = item.error ? `${item.name}: ${item.error}` : (item.descriptor ? item.name : `Uploading ${item.name}…`);
label.textContent = item.error ? `${item.name}: ${item.error}` : (item.state === 'ready' ? `Ready: ${item.name}` : `${item.state === 'retrying' ? 'Retrying' : 'Uploading'} ${item.name}…`);
label.setAttribute('role', item.error ? 'alert' : 'status');
card.appendChild(label);
const details = document.createElement('span');
Expand All @@ -96,6 +105,7 @@ export function renderImageComposer(parent, sessionId, changed) {
const remove = document.createElement('button');
remove.type = 'button'; remove.textContent = 'Remove'; remove.setAttribute('aria-label', `Remove ${item.name}`);
remove.onclick = () => {
item.state = 'removed';
item.controller?.abort();
pending.set(sessionId, imageItems(sessionId).filter(x => x !== item));
releasePreview(item);
Expand All @@ -109,7 +119,7 @@ export function isConversationImageFile(file) {
}
export async function uploadConversationImage(file, sessionId, headers, changed) {
const item = {
name: file.name, type: file.type, size: file.size,
name: file.name, type: file.type, size: file.size, state: 'queued',
previewUrl: file.type.startsWith('image/') && typeof URL.createObjectURL === 'function' ? URL.createObjectURL(file) : null,
};
item.retry = () => uploadItem(item, file, sessionId, headers, changed);
Expand All @@ -121,8 +131,8 @@ function releasePreview(item) {
item.previewUrl = null;
}
async function uploadItem(item, file, sessionId, headers, changed) {
if (item.uploading || !imageItems(sessionId).includes(item)) return false;
item.uploading = true;
if (['uploading', 'retrying', 'ready', 'removed'].includes(item.state) || !imageItems(sessionId).includes(item)) return false;
item.state = item.state === 'failed' ? 'retrying' : 'uploading';
item.controller = new AbortController();
item.error = null;
changed();
Expand All @@ -143,16 +153,23 @@ async function uploadItem(item, file, sessionId, headers, changed) {
const descriptor = result.image_attachment || (result.uploaded?.concept_id ? {...result.uploaded, filename: file.name, content_type: file.type || 'application/octet-stream', size_bytes: file.size} : null);
if (!response.ok || !descriptor) throw new Error(result.message || result.error || 'Attachment upload failed');
// Removing an in-flight upload must not restore it when the response arrives.
if (imageItems(sessionId).includes(item)) {
if (item.state !== 'removed' && imageItems(sessionId).includes(item)) {
if (typeof descriptor.concept_id !== 'string' || !/^#V#\S+$/.test(descriptor.concept_id)) throw new Error('Upload returned no valid attachment identifier');
item.descriptor = descriptor;
item.state = 'ready';
item.retry = () => Promise.resolve(true);
known.set(item.descriptor.concept_id, item);
}
releasePreview(item);
} catch (error) { item.error = String(error.message || error); }
item.uploading = false;
} catch (error) {
if (item.state !== 'removed' && item.state !== 'ready') {
item.state = 'failed';
item.error = String(error.message || error);
}
}
item.controller = null;
changed();
return Boolean(item.descriptor);
return item.state === 'ready';
}
// Both controls use the existing session-bound upload path supplied by chatTab.
export function initialiseImagePicker(upload, status) {
Expand Down
57 changes: 57 additions & 0 deletions tests/browser/attachmentLifecycle.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Local Chromium component acceptance: actual attachment module, fixture transport.
// Does not use server authentication, upload real data, or invoke a model.
process.env.PLAYWRIGHT_BROWSERS_PATH ||= '/tmp/von-playwright';
const { chromium, expect } = require('@playwright/test');
const fs = require('node:fs');
const path = require('node:path');
const output = process.argv[2] || '.run/attachment-lifecycle';
(async () => {
fs.mkdirSync(output, { recursive: true });
const browser = await chromium.launch({ headless: true });
try {
const page = await browser.newPage();
await page.route('http://attachment.test/**', route => {
if (route.request().url().endsWith('/module.js')) return route.fulfill({
contentType: 'text/javascript',
body: fs.readFileSync(path.join(__dirname, '../../src/frontend/web/von_interface/static/js/utils/conversationImages.js'), 'utf8')
});
return route.fulfill({ contentType: 'text/html', body: '<main><div id="attachments"></div><button id="send">Send</button><p id="status"></p></main>' });
});
await page.goto('http://attachment.test/');
await page.evaluate(async () => {
const api = await import('/module.js');
const root = document.getElementById('attachments');
const refresh = () => {
api.renderImageComposer(root, 'draft', refresh);
document.getElementById('send').disabled = api.imagesBlocked('draft');
document.getElementById('status').textContent = api.imagesBlocked('draft') ? api.attachmentBlockMessage('draft') : 'Ready to send';
};
window.api = api;
window.fetch = async (_, options) => {
const name = options.body.get('file').name;
if (name === 'program.pdf') return new Promise(resolve => { window.finish = resolve; });
return { ok: true, json: async () => ({ uploaded: { concept_id: '#V#poster', sha256: 'fixture-sha' } }) };
};
await api.uploadConversationImage(new File(['poster'], 'poster.pdf', { type: 'application/pdf' }), 'draft', {}, refresh);
window.slow = api.uploadConversationImage(new File(['program'], 'program.pdf', { type: 'application/pdf' }), 'draft', {}, refresh);
});
await expect(page.locator('[data-attachment-state="ready"]')).toContainText('poster.pdf');
await expect(page.locator('[data-attachment-state="uploading"]')).toContainText('program.pdf');
await expect(page.locator('#send')).toBeDisabled();
await page.getByRole('button', { name: 'Remove program.pdf', exact: true }).click();
await expect(page.locator('#send')).toBeEnabled();
await expect(page.locator('#status')).toHaveText('Ready to send');
await page.screenshot({ path: path.join(output, 'ready-after-removal.png') });
const result = await page.evaluate(async () => {
const ids = api.takeImages('draft');
finish({ ok: true, json: async () => ({ uploaded: { concept_id: '#V#late' } }) });
await slow;
return { ids, descriptors: api.descriptorsForIds(ids), remaining: api.imageItems('draft').length };
});
expect(result.ids).toEqual(['#V#poster']);
expect(result.descriptors[0]).toMatchObject({ filename: 'poster.pdf', content_type: 'application/pdf', size_bytes: 6, sha256: 'fixture-sha' });
expect(result.remaining).toBe(0);
fs.writeFileSync(path.join(output, 'result.json'), JSON.stringify({ method: 'Chromium actual attachment component with local fixture transport', ...result }, null, 2));
console.log('PASS: ready/uploading status, removal unblocks send, late completion ignored, metadata preserved');
} finally { await browser.close(); }
})().catch(error => { console.error(error); process.exitCode = 1; });
Loading
Loading