Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/comment-file-table.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@buildinternet/uploads": patch
---

Render non-media attachments (PDF, zip, text, CSV, JSON, markdown, tgz) in the managed GitHub attachments comment as a small file table (name, type, size) instead of bare list links.
13 changes: 13 additions & 0 deletions apps/api/src/comment-preview-fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,22 @@ export function previewFixtureItems(env: Env): AttachmentItem[] {
pageUrl: null,
meta,
});
// Two non-media fixtures (issue #946) so the settings preview also shows
// the file table, not just the image grid — plausible preview-base paths,
// never a real `/f/` file page (`pageUrl: null`, matching the images above).
const file = (filename: string, size: number, contentType: string): AttachmentItem => ({
key: `gh/preview/pull/0/${filename}`,
url: `${webOrigin(env)}/preview/${filename}`,
embedUrl: null,
pageUrl: null,
size,
contentType,
});
return [
item("dashboard-overview.png", "comment-dashboard", { path: "/dashboard", state: "after" }),
item("settings-before.png", "comment-settings-before", { path: "/settings", state: "before" }),
item("settings-after.png", "comment-settings-after", { path: "/settings", state: "after" }),
file("report.pdf", 1_240_000, "application/pdf"),
file("bundle.zip", 8_400_000, "application/zip"),
];
}
7 changes: 7 additions & 0 deletions apps/api/src/github-comment-render.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ const goldenCap = loadFixture("github-comment-golden-cap.json");
const goldenMeta = loadFixture("github-comment-golden-meta.json");
const goldenVideo = loadFixture("github-comment-golden-video.json");
const goldenEmpty = loadFixture("github-comment-golden-empty.json");
const goldenFiles = loadFixture("github-comment-files.json");
const goldenOptions = loadOptionsFixture("github-comment-golden-options.json");
const goldenPrivateKeys = loadPrivateKeyFixture("github-comment-golden-private-keys.json");

Expand Down Expand Up @@ -167,6 +168,12 @@ describe("attachmentsCommentBody (api copy)", () => {
);
});

it("renders non-media attachments as a file table, never bullets or overflow (issue #946)", () => {
expect(attachmentsCommentBody(goldenFiles.items, goldenFiles.galleries)).toBe(
goldenFiles.expected,
);
});

it("captions overflow rows and escapes markdown metacharacters", () => {
// 18 images exceeds MAX_INLINE_ATTACHMENT_IMAGES (16), so the last two
// collapse into the <details> list — those rows must caption too.
Expand Down
2 changes: 2 additions & 0 deletions apps/api/src/github-comment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,8 @@ async function gatherAttachments(
url: o.url,
embedUrl: o.embedUrl,
pageUrl: linkToFilePage ? o.pageUrl : null,
size: o.size,
contentType: o.contentType,
});
cursor = page.cursor ?? undefined;
} while (cursor);
Expand Down
2 changes: 2 additions & 0 deletions apps/api/src/routes/workspace-settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1389,6 +1389,8 @@ export async function commentPreviewHandler(c: Context<SettingsVars>) {
url: o.url,
embedUrl: o.embedUrl,
pageUrl: linkToFilePage ? (o.pageUrl ?? null) : null,
size: o.size,
contentType: o.contentType,
}));
let sample: "workspace" | "fixtures" = "workspace";
if (items.length === 0) {
Expand Down
81 changes: 79 additions & 2 deletions packages/comment-render/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,10 @@ export interface AttachmentItem {
videoMeta?: { durationSeconds?: number; width?: number; height?: number };
/** Server-derived pixel dimensions for an image (never client-settable). */
imageMeta?: { width?: number; height?: number };
/** Object size in bytes, when known — drives the non-media file table's Size column. */
size?: number;
/** Stored content type, when known — preferred over name-based inference for file classification and the table's Type column. */
contentType?: string;
}

/** A public gallery linked to the PR or issue whose managed comment is syncing. */
Expand Down Expand Up @@ -391,6 +395,46 @@ function formatMetaCaption(
.join(" · ");
}

/**
* Decimal-SI byte size for the non-media file table's Size column: whole
* bytes below 1000, one decimal place at KB/MB/GB and above. `"—"` when the
* size is unknown.
*/
function formatBytes(bytes: number | undefined): string {
if (bytes == null || !Number.isFinite(bytes) || bytes < 0) return "—";
if (bytes < 1000) return `${Math.round(bytes)} B`;
const units: [number, string][] = [
[1e9, "GB"],
[1e6, "MB"],
[1e3, "KB"],
];
for (const [threshold, label] of units) {
if (bytes >= threshold) return `${(bytes / threshold).toFixed(1)} ${label}`;
}
return `${Math.round(bytes)} B`;
}

/**
* Type label for the non-media file table: uppercase filename extension
* first, then the content type's subtype, then a bare "FILE" fallback.
*/
function fileTypeLabel(name: string, contentType: string | undefined): string {
const dot = name.lastIndexOf(".");
if (dot !== -1 && dot < name.length - 1) return name.slice(dot + 1).toUpperCase();
if (contentType) {
const slash = contentType.indexOf("/");
if (slash !== -1 && slash < contentType.length - 1) {
return contentType.slice(slash + 1).toUpperCase();
}
}
return "FILE";
}

/** Escape `|` so a filename can't break out of a markdown table cell. */
function escapeTableCell(s: string): string {
return s.replace(/\|/g, "\\|");
}

/** Resolved pixel width for an image site, or `null` meaning "omit the width
* attribute". `"auto"` defers to the caller's per-item heuristic (`autoPx`);
* `"full"` always omits; a number always wins. */
Expand Down Expand Up @@ -619,6 +663,10 @@ export function attachmentsCommentBody(

let inlinedImages = 0;
const overflowImages: AttachmentItem[] = [];
// Non-media attachments (PDFs, archives, text/data files) never inline and
// never overflow into the <details> link list — they render as one table
// after the image/video section instead (issue #946).
const fileItems: AttachmentItem[] = [];
for (let idx = 0; idx < sorted.length; idx++) {
if (consumedByPair.has(idx)) continue;
const item = sorted[idx];
Expand All @@ -643,8 +691,22 @@ export function attachmentsCommentBody(
const stable = item.url;
const src = item.embedUrl ?? item.url;
const link = item.pageUrl ?? stable; // click-through: file page when known, else raw
const isImage = Boolean(src) && inferContentType(name).startsWith("image/");
const isPosterVideo = Boolean(item.posterUrl) && inferContentType(name).startsWith("video/");
// "application/octet-stream" is the server's generic fallback for an
// object stored without an explicit content type — not a real signal —
// so it defers to the filename the same as an absent `contentType`.
const effectiveType =
item.contentType && item.contentType !== "application/octet-stream"
? item.contentType
: inferContentType(name);
const isImage = Boolean(src) && effectiveType.startsWith("image/");
const isPosterVideo = Boolean(item.posterUrl) && effectiveType.startsWith("video/");
if (!effectiveType.startsWith("image/") && !effectiveType.startsWith("video/")) {
// Neither an image nor a video by content type — a non-media
// attachment goes into the file table, never the bullet list or
// overflow details.
fileItems.push(item);
continue;
}
const inlines = isImage || isPosterVideo;
if (inlines && inlinedImages >= options.maxInlineImages) {
// Cap hit — defer to the collapsed overflow list below rather than
Expand Down Expand Up @@ -735,6 +797,21 @@ export function attachmentsCommentBody(
lines.push(`- ${name}${cap ? ` · ${cap}` : ""}`);
}
}
if (fileItems.length > 0) {
lines.push("| File | Type | Size |", "| --- | --- | --- |");
for (const item of fileItems) {
const name = item.key.slice(item.key.lastIndexOf("/") + 1);
const escapedName = escapeTableCell(name);
let fileCell = item.url ? `[${escapedName}](${item.url})` : escapedName;
if (item.pageUrl) fileCell += ` · [page](${item.pageUrl})`;
const cap = formatMetaCaption(item.meta, options, "markdown");
if (cap) fileCell += ` · ${cap}`;
const typeLabel = fileTypeLabel(name, item.contentType);
const sizeLabel = formatBytes(item.size);
lines.push(`| ${fileCell} | ${typeLabel} | ${sizeLabel} |`);
}
lines.push("");
}
if (overflowImages.length > 0) {
const n = overflowImages.length;
lines.push(`<details><summary>${n} more attachment${n === 1 ? "" : "s"}</summary>`, "");
Expand Down
3 changes: 2 additions & 1 deletion packages/uploads/src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -938,7 +938,7 @@ export async function syncAttachmentsComment(
);
const items: AttachmentItem[] = await ghMergedList(prefixes, undefined, async (prefix) =>
(await client.listAll({ prefix, metadata: true })).map(
({ key, url, embedUrl, pageUrl, metadata }) => {
({ key, url, embedUrl, pageUrl, size, metadata }) => {
// The list endpoint returns every metadata key; the comment
// renders only these two. Narrowing here keeps both render paths
// byte-identical.
Expand All @@ -956,6 +956,7 @@ export async function syncAttachmentsComment(
url,
embedUrl,
pageUrl,
...(size != null ? { size } : {}),
...(path || state
? { meta: { ...(path ? { path } : {}), ...(state ? { state } : {}) } }
: {}),
Expand Down
81 changes: 79 additions & 2 deletions packages/uploads/src/comment-render.generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ export interface AttachmentItem {
videoMeta?: { durationSeconds?: number; width?: number; height?: number };
/** Server-derived pixel dimensions for an image (never client-settable). */
imageMeta?: { width?: number; height?: number };
/** Object size in bytes, when known — drives the non-media file table's Size column. */
size?: number;
/** Stored content type, when known — preferred over name-based inference for file classification and the table's Type column. */
contentType?: string;
}

/** A public gallery linked to the PR or issue whose managed comment is syncing. */
Expand Down Expand Up @@ -393,6 +397,46 @@ function formatMetaCaption(
.join(" · ");
}

/**
* Decimal-SI byte size for the non-media file table's Size column: whole
* bytes below 1000, one decimal place at KB/MB/GB and above. `"—"` when the
* size is unknown.
*/
function formatBytes(bytes: number | undefined): string {
if (bytes == null || !Number.isFinite(bytes) || bytes < 0) return "—";
if (bytes < 1000) return `${Math.round(bytes)} B`;
const units: [number, string][] = [
[1e9, "GB"],
[1e6, "MB"],
[1e3, "KB"],
];
for (const [threshold, label] of units) {
if (bytes >= threshold) return `${(bytes / threshold).toFixed(1)} ${label}`;
}
return `${Math.round(bytes)} B`;
}

/**
* Type label for the non-media file table: uppercase filename extension
* first, then the content type's subtype, then a bare "FILE" fallback.
*/
function fileTypeLabel(name: string, contentType: string | undefined): string {
const dot = name.lastIndexOf(".");
if (dot !== -1 && dot < name.length - 1) return name.slice(dot + 1).toUpperCase();
if (contentType) {
const slash = contentType.indexOf("/");
if (slash !== -1 && slash < contentType.length - 1) {
return contentType.slice(slash + 1).toUpperCase();
}
}
return "FILE";
}

/** Escape `|` so a filename can't break out of a markdown table cell. */
function escapeTableCell(s: string): string {
return s.replace(/\|/g, "\\|");
}

/** Resolved pixel width for an image site, or `null` meaning "omit the width
* attribute". `"auto"` defers to the caller's per-item heuristic (`autoPx`);
* `"full"` always omits; a number always wins. */
Expand Down Expand Up @@ -621,6 +665,10 @@ export function attachmentsCommentBody(

let inlinedImages = 0;
const overflowImages: AttachmentItem[] = [];
// Non-media attachments (PDFs, archives, text/data files) never inline and
// never overflow into the <details> link list — they render as one table
// after the image/video section instead (issue #946).
const fileItems: AttachmentItem[] = [];
for (let idx = 0; idx < sorted.length; idx++) {
if (consumedByPair.has(idx)) continue;
const item = sorted[idx];
Expand All @@ -645,8 +693,22 @@ export function attachmentsCommentBody(
const stable = item.url;
const src = item.embedUrl ?? item.url;
const link = item.pageUrl ?? stable; // click-through: file page when known, else raw
const isImage = Boolean(src) && inferContentType(name).startsWith("image/");
const isPosterVideo = Boolean(item.posterUrl) && inferContentType(name).startsWith("video/");
// "application/octet-stream" is the server's generic fallback for an
// object stored without an explicit content type — not a real signal —
// so it defers to the filename the same as an absent `contentType`.
const effectiveType =
item.contentType && item.contentType !== "application/octet-stream"
? item.contentType
: inferContentType(name);
const isImage = Boolean(src) && effectiveType.startsWith("image/");
const isPosterVideo = Boolean(item.posterUrl) && effectiveType.startsWith("video/");
if (!effectiveType.startsWith("image/") && !effectiveType.startsWith("video/")) {
// Neither an image nor a video by content type — a non-media
// attachment goes into the file table, never the bullet list or
// overflow details.
fileItems.push(item);
continue;
}
const inlines = isImage || isPosterVideo;
if (inlines && inlinedImages >= options.maxInlineImages) {
// Cap hit — defer to the collapsed overflow list below rather than
Expand Down Expand Up @@ -737,6 +799,21 @@ export function attachmentsCommentBody(
lines.push(`- ${name}${cap ? ` · ${cap}` : ""}`);
}
}
if (fileItems.length > 0) {
lines.push("| File | Type | Size |", "| --- | --- | --- |");
for (const item of fileItems) {
const name = item.key.slice(item.key.lastIndexOf("/") + 1);
const escapedName = escapeTableCell(name);
let fileCell = item.url ? `[${escapedName}](${item.url})` : escapedName;
if (item.pageUrl) fileCell += ` · [page](${item.pageUrl})`;
const cap = formatMetaCaption(item.meta, options, "markdown");
if (cap) fileCell += ` · ${cap}`;
const typeLabel = fileTypeLabel(name, item.contentType);
const sizeLabel = formatBytes(item.size);
lines.push(`| ${fileCell} | ${typeLabel} | ${sizeLabel} |`);
}
lines.push("");
}
if (overflowImages.length > 0) {
const n = overflowImages.length;
lines.push(`<details><summary>${n} more attachment${n === 1 ? "" : "s"}</summary>`, "");
Expand Down
20 changes: 12 additions & 8 deletions packages/uploads/test/github.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ describe("ghMetadataForBranch", () => {
});

describe("attachmentsCommentBody", () => {
it("starts with the marker and renders images with a width cap, other files as links", () => {
it("starts with the marker and renders images with a width cap, other files as a table (issue #946)", () => {
const body = attachmentsCommentBody([
{ key: "gh/o/r/pull/1/notes.txt", url: "https://x.test/gh/o/r/pull/1/notes.txt" },
{ key: "gh/o/r/pull/1/after.png", url: "https://x.test/gh/o/r/pull/1/after.png" },
Expand All @@ -250,11 +250,12 @@ describe("attachmentsCommentBody", () => {
'<a href="https://x.test/gh/o/r/pull/1/after.png"><img width="720" alt="after.png" src="https://x.test/gh/o/r/pull/1/after.png"></a>',
);
expect(body).not.toContain("![after.png]");
expect(body).toContain("- [notes.txt](https://x.test/gh/o/r/pull/1/notes.txt)");
expect(body).toContain("| File | Type | Size |");
expect(body).toContain("| [notes.txt](https://x.test/gh/o/r/pull/1/notes.txt) | TXT | — |");
expect(body).toContain('<a href="https://uploads.sh">uploads.sh</a>');
});

it("renders pdf, json, and zip items as link bullets, never as embeds", () => {
it("renders pdf, json, and zip items as a file table, never as embeds or bullets (issue #946)", () => {
const body = attachmentsCommentBody([
{ key: "gh/o/r/pull/1/lighthouse.json", url: "https://x.test/lighthouse.json" },
{
Expand All @@ -264,9 +265,12 @@ describe("attachmentsCommentBody", () => {
},
{ key: "gh/o/r/pull/1/dist.zip", url: "https://x.test/dist.zip" },
]);
expect(body).toContain("- [lighthouse.json](https://x.test/lighthouse.json)");
expect(body).toContain("- [report.pdf](https://uploads.sh/f/w/report.pdf)");
expect(body).toContain("- [dist.zip](https://x.test/dist.zip)");
expect(body).toContain("| [lighthouse.json](https://x.test/lighthouse.json) | JSON | — |");
expect(body).toContain(
"| [report.pdf](https://x.test/report.pdf) · [page](https://uploads.sh/f/w/report.pdf) | PDF | — |",
);
expect(body).toContain("| [dist.zip](https://x.test/dist.zip) | ZIP | — |");
expect(body).not.toContain("- [lighthouse.json]");
expect(body).not.toContain("<img");
});

Expand Down Expand Up @@ -304,9 +308,9 @@ describe("attachmentsCommentBody", () => {
expect(a.indexOf("a.png")).toBeLessThan(a.indexOf("b.png"));
});

it("lists items without a url as plain names", () => {
it("lists items without a url as a plain, unlinked table row", () => {
const body = attachmentsCommentBody([{ key: "gh/o/r/pull/1/x.bin", url: null }]);
expect(body).toContain("- x.bin");
expect(body).toContain("| x.bin | BIN | — |");
});

it("renders a distinct, safely escaped Galleries section without attachments", () => {
Expand Down
Loading
Loading