perf(twenty-server): browser-cache picture file responses (avatars/logos) (#22166)
Fixes #22163. ## Why Picture file responses (`GET /file/:fileFolder/:id`, `FileController.getFileById`) set no `Cache-Control` header, so the browser re-fetches the same avatar/picture on every render. When one member's avatar appears many times on a page (e.g. a record table or Kanban where that member owns many rows), this fires dozens of parallel GETs for the identical image; the browser cancels the redundant in-flight ones, and the server logs each client-aborted stream as `Error streaming file from storage`. Picture files are content-addressed by an immutable file id — changing an avatar or logo mints a new file id (and therefore a new URL) — so the bytes at any given URL never change and can be cached aggressively. ## What - `setFileResponseHeaders` now adds `Cache-Control: private, max-age=86400, immutable` for the picture folders (`CorePicture`, `ProfilePicture`, `WorkspaceLogo`, `PersonPicture`); `getFileById` passes the `fileFolder` through. - Scoped to picture folders so non-image files (attachments, tarballs, source, …) are not cached past a permission/visibility change. - `private` because files are served behind a per-workspace file token; `immutable` + the content-addressed id gives automatic cache-busting when the picture changes. ## Tests - Unit tests for `setFileResponseHeaders`: header is set for each picture folder, and not set for non-picture folders or when no folder is provided. - Controller test asserts the header on a `CorePicture` stream response. <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/twentyhq/twenty/pull/22166?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> --------- Co-authored-by: Charles Bochet <charles@twenty.com> Co-authored-by: Félix Malfait <felix.malfait@gmail.com>
This commit is contained in:
+4
@@ -151,6 +151,10 @@ describe('FileController', () => {
|
||||
'Content-Disposition',
|
||||
'inline',
|
||||
);
|
||||
expect(mockResponse.setHeader).toHaveBeenCalledWith(
|
||||
'Cache-Control',
|
||||
'private, max-age=86400, immutable',
|
||||
);
|
||||
expect(mockPipeline).toHaveBeenCalledWith(mockStream, mockResponse);
|
||||
});
|
||||
|
||||
|
||||
@@ -152,7 +152,7 @@ export class FileController {
|
||||
return res.redirect(fileResponse.presignedUrl);
|
||||
}
|
||||
|
||||
setFileResponseHeaders(res, fileResponse.mimeType);
|
||||
setFileResponseHeaders(res, fileResponse.mimeType, fileFolder);
|
||||
|
||||
try {
|
||||
await pipeline(fileResponse.stream, res);
|
||||
|
||||
@@ -11,6 +11,8 @@ import { type WorkspaceEntity } from 'src/engine/core-modules/workspace/workspac
|
||||
|
||||
@Injectable()
|
||||
export class FileUrlService {
|
||||
private readonly inflightFileUrlSignings = new Map<string, Promise<string>>();
|
||||
|
||||
constructor(
|
||||
private readonly jwtWrapperService: JwtWrapperService,
|
||||
private readonly twentyConfigService: TwentyConfigService,
|
||||
@@ -39,10 +41,39 @@ export class FileUrlService {
|
||||
workspaceId: string;
|
||||
fileFolder: FileFolder;
|
||||
}): Promise<string> {
|
||||
const fileTokenExpiresIn = this.twentyConfigService.get(
|
||||
'FILE_TOKEN_EXPIRES_IN',
|
||||
);
|
||||
const signingCacheKey = `${workspaceId}:${fileFolder}:${fileId}`;
|
||||
const inflightSigning = this.inflightFileUrlSignings.get(signingCacheKey);
|
||||
|
||||
if (isDefined(inflightSigning)) {
|
||||
return inflightSigning;
|
||||
}
|
||||
|
||||
const signing = (async () => {
|
||||
try {
|
||||
return await this.buildSignedFileUrl({
|
||||
fileId,
|
||||
workspaceId,
|
||||
fileFolder,
|
||||
});
|
||||
} finally {
|
||||
this.inflightFileUrlSignings.delete(signingCacheKey);
|
||||
}
|
||||
})();
|
||||
|
||||
this.inflightFileUrlSignings.set(signingCacheKey, signing);
|
||||
|
||||
return signing;
|
||||
}
|
||||
|
||||
private async buildSignedFileUrl({
|
||||
fileId,
|
||||
workspaceId,
|
||||
fileFolder,
|
||||
}: {
|
||||
fileId: string;
|
||||
workspaceId: string;
|
||||
fileFolder: FileFolder;
|
||||
}): Promise<string> {
|
||||
const payload: FileTokenJwtPayload = {
|
||||
workspaceId,
|
||||
fileId,
|
||||
@@ -51,7 +82,7 @@ export class FileUrlService {
|
||||
};
|
||||
|
||||
const token = await this.jwtWrapperService.signAsyncOrThrow(payload, {
|
||||
expiresIn: fileTokenExpiresIn,
|
||||
expiresIn: this.twentyConfigService.get('FILE_TOKEN_EXPIRES_IN'),
|
||||
});
|
||||
|
||||
const serverUrl = this.twentyConfigService.get('SERVER_URL');
|
||||
|
||||
+46
@@ -1,3 +1,5 @@
|
||||
import { FileFolder } from 'twenty-shared/types';
|
||||
|
||||
import { getContentDisposition } from 'src/engine/core-modules/file/utils/get-content-disposition.utils';
|
||||
import { setFileResponseHeaders } from 'src/engine/core-modules/file/utils/set-file-response-headers.utils';
|
||||
|
||||
@@ -72,6 +74,50 @@ describe('setFileResponseHeaders', () => {
|
||||
);
|
||||
},
|
||||
);
|
||||
|
||||
it('should not set Cache-Control when no fileFolder is provided', () => {
|
||||
const res = createMockResponse();
|
||||
|
||||
setFileResponseHeaders(res as any, 'image/png');
|
||||
|
||||
expect(res.setHeader).not.toHaveBeenCalledWith(
|
||||
'Cache-Control',
|
||||
expect.anything(),
|
||||
);
|
||||
});
|
||||
|
||||
it('should set an immutable Cache-Control for the CorePicture folder', () => {
|
||||
const res = createMockResponse();
|
||||
|
||||
setFileResponseHeaders(res as any, 'image/png', FileFolder.CorePicture);
|
||||
|
||||
expect(res.setHeader).toHaveBeenCalledWith(
|
||||
'Cache-Control',
|
||||
'private, max-age=86400, immutable',
|
||||
);
|
||||
});
|
||||
|
||||
it.each([
|
||||
FileFolder.ProfilePicture,
|
||||
FileFolder.WorkspaceLogo,
|
||||
FileFolder.PersonPicture,
|
||||
FileFolder.FilesField,
|
||||
FileFolder.Attachment,
|
||||
FileFolder.Workflow,
|
||||
FileFolder.PublicAsset,
|
||||
])(
|
||||
'should not set Cache-Control for non-cacheable folder %s',
|
||||
(fileFolder) => {
|
||||
const res = createMockResponse();
|
||||
|
||||
setFileResponseHeaders(res as any, 'image/png', fileFolder);
|
||||
|
||||
expect(res.setHeader).not.toHaveBeenCalledWith(
|
||||
'Cache-Control',
|
||||
expect.anything(),
|
||||
);
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
describe('getContentDisposition', () => {
|
||||
|
||||
+19
-1
@@ -1,11 +1,29 @@
|
||||
import { type Response } from 'express';
|
||||
|
||||
import { FileFolder } from 'twenty-shared/types';
|
||||
import { isDefined } from 'twenty-shared/utils';
|
||||
|
||||
import { getContentDisposition } from 'src/engine/core-modules/file/utils/get-content-disposition.utils';
|
||||
|
||||
export const setFileResponseHeaders = (res: Response, mimeType: string) => {
|
||||
const CACHEABLE_PICTURE_FILE_FOLDERS: FileFolder[] = [FileFolder.CorePicture];
|
||||
|
||||
const PICTURE_CACHE_CONTROL = 'private, max-age=86400, immutable';
|
||||
|
||||
export const setFileResponseHeaders = (
|
||||
res: Response,
|
||||
mimeType: string,
|
||||
fileFolder?: FileFolder,
|
||||
) => {
|
||||
const contentType = mimeType || 'application/octet-stream';
|
||||
|
||||
res.setHeader('Content-Type', contentType);
|
||||
res.setHeader('X-Content-Type-Options', 'nosniff');
|
||||
res.setHeader('Content-Disposition', getContentDisposition(contentType));
|
||||
|
||||
if (
|
||||
isDefined(fileFolder) &&
|
||||
CACHEABLE_PICTURE_FILE_FOLDERS.includes(fileFolder)
|
||||
) {
|
||||
res.setHeader('Cache-Control', PICTURE_CACHE_CONTROL);
|
||||
}
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user