mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-21 23:26:34 +01:00
🩺 refactor: Surface Descriptive OCR Error Messages to Client (#12344)
* fix: pass along error message when OCR fails Right now, if OCR fails, it just says "Error processing file" which isn't very helpful. The `error.message` does has helpful information in it, but our filter wasn't including the right case to pass it along. Now it does! * fix: extract shared upload error filter, apply to images route The 'Unable to extract text from' error was only allowlisted in the files route but not the images route, which also calls processAgentFileUpload. Extract the duplicated error filter logic into a shared resolveUploadErrorMessage utility in packages/api so both routes stay in sync. --------- Co-authored-by: Dan Lew <daniel@mightyacorn.com>
This commit is contained in:
parent
676d297cb4
commit
365a0dc0f6
4 changed files with 91 additions and 25 deletions
|
|
@ -2,7 +2,7 @@ const fs = require('fs').promises;
|
|||
const express = require('express');
|
||||
const { EnvVar } = require('@librechat/agents');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const { verifyAgentUploadPermission } = require('@librechat/api');
|
||||
const { verifyAgentUploadPermission, resolveUploadErrorMessage } = require('@librechat/api');
|
||||
const {
|
||||
Time,
|
||||
isUUID,
|
||||
|
|
@ -394,21 +394,9 @@ router.post('/', async (req, res) => {
|
|||
|
||||
return await processAgentFileUpload({ req, res, metadata });
|
||||
} catch (error) {
|
||||
let message = 'Error processing file';
|
||||
const message = resolveUploadErrorMessage(error);
|
||||
logger.error('[/files] Error processing file:', error);
|
||||
|
||||
if (error.message?.includes('file_ids')) {
|
||||
message += ': ' + error.message;
|
||||
}
|
||||
|
||||
if (
|
||||
error.message?.includes('Invalid file format') ||
|
||||
error.message?.includes('No OCR result') ||
|
||||
error.message?.includes('exceeds token limit')
|
||||
) {
|
||||
message = error.message;
|
||||
}
|
||||
|
||||
try {
|
||||
await fs.unlink(req.file.path);
|
||||
cleanup = false;
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@ const path = require('path');
|
|||
const fs = require('fs').promises;
|
||||
const express = require('express');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const { verifyAgentUploadPermission } = require('@librechat/api');
|
||||
const { verifyAgentUploadPermission, resolveUploadErrorMessage } = require('@librechat/api');
|
||||
const { isAssistantsEndpoint } = require('librechat-data-provider');
|
||||
const {
|
||||
processAgentFileUpload,
|
||||
|
|
@ -43,15 +43,7 @@ router.post('/', async (req, res) => {
|
|||
// TODO: delete remote file if it exists
|
||||
logger.error('[/files/images] Error processing file:', error);
|
||||
|
||||
let message = 'Error processing file';
|
||||
|
||||
if (
|
||||
error.message?.includes('Invalid file format') ||
|
||||
error.message?.includes('No OCR result') ||
|
||||
error.message?.includes('exceeds token limit')
|
||||
) {
|
||||
message = error.message;
|
||||
}
|
||||
const message = resolveUploadErrorMessage(error);
|
||||
|
||||
try {
|
||||
const filepath = path.join(
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import { sanitizeFilename } from './files';
|
||||
import { sanitizeFilename, resolveUploadErrorMessage } from './files';
|
||||
|
||||
jest.mock('node:crypto', () => {
|
||||
const actualModule = jest.requireActual('node:crypto');
|
||||
|
|
@ -52,6 +52,59 @@ describe('sanitizeFilename', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('resolveUploadErrorMessage', () => {
|
||||
test('returns default message for null error', () => {
|
||||
expect(resolveUploadErrorMessage(null)).toBe('Error processing file');
|
||||
});
|
||||
|
||||
test('returns default message for undefined error', () => {
|
||||
expect(resolveUploadErrorMessage(undefined)).toBe('Error processing file');
|
||||
});
|
||||
|
||||
test('returns default message when error has no message property', () => {
|
||||
expect(resolveUploadErrorMessage({})).toBe('Error processing file');
|
||||
});
|
||||
|
||||
test('returns default message for unrecognized error', () => {
|
||||
expect(resolveUploadErrorMessage({ message: 'ENOENT: no such file or directory' })).toBe(
|
||||
'Error processing file',
|
||||
);
|
||||
});
|
||||
|
||||
test('prepends default message for file_ids errors', () => {
|
||||
expect(resolveUploadErrorMessage({ message: 'max file_ids reached' })).toBe(
|
||||
'Error processing file: max file_ids reached',
|
||||
);
|
||||
});
|
||||
|
||||
test('surfaces "Invalid file format" errors', () => {
|
||||
expect(resolveUploadErrorMessage({ message: 'Invalid file format: .xyz' })).toBe(
|
||||
'Invalid file format: .xyz',
|
||||
);
|
||||
});
|
||||
|
||||
test('surfaces "exceeds token limit" errors', () => {
|
||||
expect(resolveUploadErrorMessage({ message: 'Content exceeds token limit' })).toBe(
|
||||
'Content exceeds token limit',
|
||||
);
|
||||
});
|
||||
|
||||
test('surfaces "Unable to extract text from" errors', () => {
|
||||
const msg = 'Unable to extract text from "doc.pdf". The document may be image-based.';
|
||||
expect(resolveUploadErrorMessage({ message: msg })).toBe(msg);
|
||||
});
|
||||
|
||||
test('accepts a custom default message', () => {
|
||||
expect(resolveUploadErrorMessage(null, 'Custom default')).toBe('Custom default');
|
||||
});
|
||||
|
||||
test('uses custom default in file_ids prepend', () => {
|
||||
expect(resolveUploadErrorMessage({ message: 'file_ids limit' }, 'Upload failed')).toBe(
|
||||
'Upload failed: file_ids limit',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('sanitizeFilename with real crypto', () => {
|
||||
// Temporarily unmock crypto for these tests
|
||||
beforeAll(() => {
|
||||
|
|
|
|||
|
|
@ -3,6 +3,39 @@ import crypto from 'node:crypto';
|
|||
import { createReadStream } from 'fs';
|
||||
import { readFile, stat } from 'fs/promises';
|
||||
|
||||
const USER_FACING_UPLOAD_ERRORS = [
|
||||
'Invalid file format',
|
||||
'exceeds token limit',
|
||||
'Unable to extract text from',
|
||||
] as const;
|
||||
|
||||
/**
|
||||
* Resolves a user-facing error message from a file upload error.
|
||||
* Returns the error's own message if it matches a known user-facing pattern,
|
||||
* otherwise returns the default message.
|
||||
*/
|
||||
export function resolveUploadErrorMessage(
|
||||
error: { message?: string } | null | undefined,
|
||||
defaultMessage = 'Error processing file',
|
||||
): string {
|
||||
const errorMessage = error?.message;
|
||||
if (!errorMessage) {
|
||||
return defaultMessage;
|
||||
}
|
||||
|
||||
if (errorMessage.includes('file_ids')) {
|
||||
return `${defaultMessage}: ${errorMessage}`;
|
||||
}
|
||||
|
||||
for (const fragment of USER_FACING_UPLOAD_ERRORS) {
|
||||
if (errorMessage.includes(fragment)) {
|
||||
return errorMessage;
|
||||
}
|
||||
}
|
||||
|
||||
return defaultMessage;
|
||||
}
|
||||
|
||||
/**
|
||||
* Sanitize a filename by removing any directory components, replacing non-alphanumeric characters
|
||||
* @param inputName
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue