Unify media storage around Medium#286
Conversation
Add the shared Medium model and migrate note, article, and account avatar media to relation tables backed by FlyDrive keys. Keep post media for remote ActivityPub attachments separate. Replace the old uploadMedia API with createMedium, streaming upload session mutations, and article draft media attachment. Update legacy web and web-next avatar/article upload paths to use the new GraphQL/model APIs. Assisted-by: Codex:gpt-5.5
Remove an invalid ArticleDraft column selection from the contentHtml resolver. The preview only needs the draft content and medium relation data to render hp-medium references. Assisted-by: Codex:gpt-5.5
Pass article source medium URLs into the legacy article renderer so migrated hp-medium placeholders render as public image URLs. Assisted-by: Codex:gpt-5.5
Let article creation persist referenced medium mappings before rendering the post or sending ActivityPub Create. Both GraphQL and legacy draft publish now pass draft media through the shared model path. Assisted-by: Codex:gpt-5.5
Make avatarMedium a required nullable relation for actor sync inputs and load it at note and session call sites. This prevents note refresh paths from replacing uploaded avatar URLs with fallback images. Assisted-by: Codex:gpt-5.5
Apply article media URL replacements recursively during the unified medium migration. This converts every matching URL for multi-image drafts and article contents instead of relying on UPDATE ... FROM row selection. Assisted-by: Codex:gpt-5.5
Apply the existing avatar crop and size cap before storing avatar media from legacy uploads, deprecated avatarUrl updates, and avatarMediumId assignments. Add the missing Drizzle snapshot for the unified medium migration so future schema diffs have the right baseline. Assisted-by: Codex:gpt-5.5
Drop the per-note medium uniqueness constraint so the same deduplicated Medium can appear at multiple indexes with distinct alt text. Add a migration, snapshot, and regression test for repeated note attachments. Assisted-by: Codex:gpt-5.5
Validate proxy upload Content-Length before reading the request body so fallback uploads cannot buffer unbounded chunked data. Cover both the missing-length rejection path and a valid exact-length upload. Assisted-by: Codex:gpt-5.5
Rename GraphQL DateTime fields that used an At suffix to match the schema convention. Keep the internal model property names unchanged and map them at the GraphQL boundary. Assisted-by: Codex:gpt-5.5
Add a Sha256 GraphQL scalar for lowercase hex-encoded SHA-256 digests and expose Medium.contentHash through it instead of String. Update both GraphQL codegen configurations so clients continue to generate cleanly. Assisted-by: Codex:gpt-5.5
Add moderator-only orphan media status and deletion APIs, plus manual cleanup pages in both web stacks. The cleanup keeps a 24-hour grace period and only deletes media that are no longer referenced by accounts, notes, article drafts, or article sources. Assisted-by: Codex:gpt-5.5
|
@codex review |
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughImplements a unified Medium subsystem: DB enum/table and join tables; migrates existing media and avatars into Mediums; adds media creation/upload APIs (streaming session + proxy), GraphQL types/mutations/scalar, orphan-media admin flows, model wiring, frontend integration, and extensive tests and translations. ChangesUnified Medium System
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Web Client
participant GraphQL as GraphQL Server
participant DB as Database
participant Disk as Disk Storage
User->>Client: Upload avatar image / crop
Client->>GraphQL: createMedium(dataUrl) or startMediumUpload()
GraphQL->>Disk: write preprocessed bytes
Disk-->>GraphQL: key/url
GraphQL->>DB: insert/get Medium (key,type,hash,dims)
DB-->>GraphQL: Medium id/uuid
GraphQL-->>Client: medium payload { uuid, url, contentHash, width, height }
Client->>GraphQL: updateAccount(avatarMediumId)
GraphQL->>DB: update account.avatar_medium_id
DB-->>GraphQL: updated account
GraphQL->>GraphQL: syncActorFromAccount with avatarMedium
GraphQL-->>Client: updated account/actor
sequenceDiagram
actor Author
participant Client as Web Client
participant GraphQL as GraphQL Server
participant DB as Database
participant Disk as Disk Storage
Author->>Client: Create article with remote media
Client->>GraphQL: startMediumUpload(contentType, contentLength)
GraphQL->>DB: create upload session (TTL)
DB-->>GraphQL: uploadId, headers
Client->>GraphQL: PUT upload bytes -> handleMediumUploadProxy
GraphQL->>Disk: store session bytes
GraphQL-->>Client: 204
Client->>GraphQL: finishMediumUpload(uploadId)
GraphQL->>Disk: read bytes, preprocess, store
GraphQL->>DB: insert/get Medium
DB-->>GraphQL: Medium id
GraphQL->>DB: attach medium to draft via article_draft_medium
GraphQL-->>Client: draft saved with medium mapping
sequenceDiagram
actor Admin
participant UI as Admin UI
participant GraphQL as GraphQL Server
participant DB as Database
participant Disk as Disk Storage
Admin->>UI: open /admin/media
UI->>GraphQL: orphanMediaStatus
GraphQL->>DB: SELECT mediums with no refs older than cutoff
DB-->>GraphQL: count, cutoffDate
GraphQL-->>UI: status
Admin->>UI: click Delete Orphan Media
UI->>GraphQL: deleteOrphanMedia
GraphQL->>DB: SELECT orphan mediums
DB-->>GraphQL: list(keys)
loop per medium
GraphQL->>Disk: delete file by key
Disk-->>GraphQL: success or error
end
GraphQL->>DB: DELETE orphan medium rows
GraphQL-->>UI: deletion results (deletedCount, failedDiskDeletes)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a unified media management system, centralizing image handling for avatars, notes, and articles into a new medium table. The implementation includes database migrations, model refactoring, and new GraphQL mutations for media uploads and attachments, supported by a new upload proxy and an admin maintenance page for orphan media. Feedback indicates that the InvalidInputError class is used throughout graphql/post.ts without being imported, which will lead to errors.
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/postgres.ts (1)
314-325:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
createTestDisk()stub —delete()doesn't accept a key and doesn't remove files.The in-memory disk stub's
delete()method diverges from theDiskcontract. All production calls pass a key argument (disk.delete(key)in web/og.ts:87, graphql/post.ts:2210, models/admin.ts:316), but the test stub ignores it and doesn't remove anything from thefilesmap. Tests exercising orphan-media cleanup will think a file was deleted but a subsequentgetBytes(key)would still return the bytes, hiding real bugs in cleanup paths.(Note:
getBytesbehavior is correct — it throws on missing keys, matching flydrive's documented contract.)Fix: Accept key and actually remove from map
put(key: string, contents: Uint8Array) { files.set(key, contents); return Promise.resolve(undefined); }, - delete() { + delete(key: string) { + files.delete(key); return Promise.resolve(undefined); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/postgres.ts` around lines 314 - 325, The in-memory disk stub created by createTestDisk() has a delete() method that currently takes no arguments and doesn't remove entries from the files map; update delete() to accept a key parameter (key: string) and call files.delete(key) before resolving so it matches the Disk contract used by disk.delete(key) callers (see getBytes and put for existing behaviors) and ensures getBytes(key) will throw after deletion.federation/objects.ts (1)
57-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't emit raw
hp-medium:placeholders inArticle.source.
renderMarkup(..., { mediumUrls })fixes the rendered HTML, butSource.contentstill comes from the original markdown, and the...contentspread afterrenderMarkup(...)would overwrite any transformed source text returned there. ActivityPub consumers that readsource.contentwill still see unresolvedhp-medium:tokens.Also applies to: 102-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@federation/objects.ts` around lines 57 - 64, The mapping over articleSource.contents currently spreads the original content object after awaiting renderMarkup, so any transformed source text (resolved from hp-medium: by renderMarkup) gets overwritten and raw hp-medium: tokens remain in Article.source.content; change the merge order so the properties returned by await renderMarkup(ctx, content.content, { docId: articleSource.id, kv: ctx.data.kv, mediumUrls }) take precedence (or explicitly set the source/content field from the render result) instead of being overwritten by ...content, and apply the same fix to the similar block referenced at lines 102-105; locate this in the articleSource.contents map and ensure the final object uses the rendered content value for source/content.models/post.ts (1)
349-358:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the stored medium MIME type here.
Medium.typecan now beimage/gif,image/jpeg,image/png, orimage/webp, but this path rewrites every mirrored note attachment toimage/webp. That mislabels migrated/imported media and can break consumers that key behavior off the attachment type.Proposed fix
await Promise.all(noteSource.media.map(async (medium) => ({ postId: post.id, index: medium.index, - type: "image/webp" as const, + type: medium.medium.type, url: await disk.getUrl(medium.medium.key), alt: medium.alt, width: medium.medium.width, height: medium.medium.height, }))),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/post.ts` around lines 349 - 358, The insert into postMediumTable currently hardcodes type to "image/webp"; change it to use the stored medium.type from noteSource.media (e.g., medium.type) when building the object for insertion, and optionally validate or fallback if medium.type is missing/unsupported (keep disk.getUrl(medium.medium.key), postId, index, alt, width, height as-is). Update the mapping that builds the values (the Promise.all over noteSource.media) so the inserted record's type field reflects Medium.type instead of the literal "image/webp".graphql/schema.graphql (1)
658-675:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the old
*Atfields as deprecated aliases.Renaming
lastRegeneratedAt→lastRegeneratedandregeneratedAt→regeneratedremoves fields from the public schema, which will break existing queries and persisted operations. Add the new names, but keep the old ones temporarily with@deprecatedso clients can migrate safely.Suggested schema shape
type InvitationRegenerationStatus { + lastRegeneratedAt: DateTime `@deprecated`(reason: "Use lastRegenerated instead.") lastRegenerated: DateTime } type RegenerateInvitationsPayload { + regeneratedAt: DateTime! `@deprecated`(reason: "Use regenerated instead.") regenerated: DateTime! }Also applies to: 1468-1476
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graphql/schema.graphql` around lines 658 - 675, The schema removed legacy "*At" fields which breaks existing clients; add back deprecated alias fields alongside the new names: in type InvitationRegenerationStatus keep the new fields cutoffDate, eligibleAccountsCount, lastRegenerated (DateTime), and topThirdCount, and add deprecated aliases lastRegeneratedAt: DateTime `@deprecated`(reason: "Use `lastRegenerated`") and regeneratedAt or regenerated? (match the other type's rename) as deprecated aliases for the corresponding new fields; do the same for the other occurrence referenced (lines ~1468-1476) so both types expose the new field names plus the old "*At" fields marked with `@deprecated` and identical types/descriptions to allow a migration path for clients.
🧹 Nitpick comments (8)
web-next/src/locales/zh-CN/messages.po (1)
1750-1752: 💤 Low valueSlightly unnatural phrasing — "已足够旧" is a direct calque of "old enough"
"已足够旧" reads as awkward in Chinese. Consider a more idiomatic rephrasing:
✏️ Suggested revision
-msgstr "移除已足够旧且不再附加到头像、帖子、文章草稿或文章的已存储媒体。" +msgstr "移除已超过保留期且不再附加到头像、帖子、文章草稿或文章的媒体。"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/src/locales/zh-CN/messages.po` around lines 1750 - 1752, The Chinese translation for the msgid in src/routes/(root)/admin/media.tsx:163 uses an awkward literal "已足够旧"; update the corresponding msgstr to a more idiomatic phrase such as replacing "已足够旧且不再附加到头像、帖子、文章草稿或文章的已存储媒体。" with a clearer wording like "移除已过期且不再附着于头像、帖子、文章草稿或文章的已存储媒体。" — edit the msgstr for that msgid in messages.po to use "已过期" (or another natural alternative) and keep the rest of the sentence structure intact.web-next/src/routes/(root)/[handle]/settings/index.tsx (1)
283-287: ⚡ Quick winUse LogTape in the new avatar upload failure path.
This branch adds another raw
console.error, which loses the structured context the rest ofweb-nextexpects for production debugging. Please route this through LogTape and include the save context instead.As per coding guidelines, use structured logging via LogTape and include context in error details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-next/src/routes/`(root)/[handle]/settings/index.tsx around lines 283 - 287, The catch block that currently calls console.error(error) after createMediumFromDataUrl(pendingAvatarUrl) should use the project's structured logger LogTape and include the current save context; replace the raw console.error with a LogTape error call that logs the caught error plus structured fields such as the save object/identifier and pendingAvatarUrl so the failure in createMediumFromDataUrl and the avatar upload (avatarMediumId) path is recorded with proper context.models/account.ts (1)
125-147: ⚡ Quick winExpose
avatarMediumingetAccountByUsername()’s return type.Both queries now load
avatarMedium, but the explicit return type still erases it. That makes the typed API drift from the actual result shape and forces downstream casts if callers need the new relation.Possible fix
): Promise< | Account & { actor: Actor & { successor: Actor | null }; + avatarMedium: Medium | null; emails: AccountEmail[]; links: AccountLink[]; } | undefined > {As per coding guidelines, use explicit typing for complex return types in TypeScript.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/account.ts` around lines 125 - 147, The function getAccountByUsername currently queries avatarMedium but its explicit return type omits that relation; update getAccountByUsername's declared return type to include the avatarMedium relation (matching the type returned by db.query.accountTable.findFirst with with: { avatarMedium: true }) so callers get the correct shape without casts; locate getAccountByUsername in models/account.ts and extend its return/interface to include avatarMedium (nullable if DB allows null) or use the generated query result type that includes with: { avatarMedium: true } for accuracy.drizzle/0098_unified_medium.sql (1)
30-58: ⚡ Quick winAdd indexes for the new foreign-key columns.
PostgreSQL does not auto-index FKs. The orphan-media status/delete paths and any reverse lookups from
mediumwill otherwise scanaccount,note_source_medium,article_draft_medium, andarticle_source_medium.Suggested indexes
CREATE TABLE IF NOT EXISTS "article_source_medium" ( "article_source_id" uuid NOT NULL, "key" text NOT NULL, "medium_id" uuid NOT NULL, "created" timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL, CONSTRAINT "article_source_medium_article_source_id_key_pk" PRIMARY KEY("article_source_id","key") ); --> statement-breakpoint ALTER TABLE "account" ADD COLUMN "avatar_medium_id" uuid; +--> statement-breakpoint +CREATE INDEX IF NOT EXISTS "account_avatar_medium_id_idx" + ON "account" ("avatar_medium_id"); +--> statement-breakpoint +CREATE INDEX IF NOT EXISTS "note_source_medium_medium_id_idx" + ON "note_source_medium" ("medium_id"); +--> statement-breakpoint +CREATE INDEX IF NOT EXISTS "article_draft_medium_medium_id_idx" + ON "article_draft_medium" ("medium_id"); +--> statement-breakpoint +CREATE INDEX IF NOT EXISTS "article_source_medium_medium_id_idx" + ON "article_source_medium" ("medium_id"); --> statement-breakpoint DO $$ BEGINAlso applies to: 60-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drizzle/0098_unified_medium.sql` around lines 30 - 58, Add b-tree indexes on the new foreign-key columns to avoid full-table scans: create an index on note_source_medium("medium_id"), on article_draft_medium("medium_id"), and on article_source_medium("medium_id"); if there is a medium_id FK on account, add an index on account("medium_id") as well. Use clear, unique names (e.g. note_source_medium_medium_id_idx, article_draft_medium_medium_id_idx, article_source_medium_medium_id_idx, account_medium_id_idx) and create them alongside the CREATE TABLE statements so reverse lookups and orphan-media/delete paths use the index.web/routes/_app.tsx (1)
73-78: ⚡ Quick winHoist
Mediumto the existing type import block.The inline
import("@hackerspub/models/schema").Mediumis inconsistent with the surrounding style —AccountandAccountEmailare already imported from the same module via a top-leveltypeimport. PullingMediumup makes the local type alias easier to read.♻️ Proposed import hoist
import { type Account, type AccountEmail, articleDraftTable, + type Medium, } from "@hackerspub/models/schema";- let account: - | Account & { - emails: AccountEmail[]; - avatarMedium?: import("@hackerspub/models/schema").Medium | null; - } - | undefined = undefined; + let account: + | Account & { + emails: AccountEmail[]; + avatarMedium?: Medium | null; + } + | undefined = undefined;As per coding guidelines: "Use
typekeyword for type imports when appropriate in TypeScript".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/routes/_app.tsx` around lines 73 - 78, The inline import for Medium should be hoisted to the existing top-level type imports: add Medium to the existing type import list that already includes Account and AccountEmail, then update the local variable type annotation for account (the union type using Account & { emails: AccountEmail[]; avatarMedium?: import("@hackerspub/models/schema").Medium | null; }) to reference Medium directly (avatarMedium?: Medium | null). Modify the top-level type import and the account declaration to use the imported Medium symbol.graphql/medium-upload.test.ts (1)
43-74: ⚡ Quick winConsider adding a Content-Length mismatch test.
The two paths covered (missing + exact match) leave a gap: a declared
Content-Lengththat disagrees with the actual byte count (either short-write or over-read). IfhandleMediumUploadProxyenforces the declared length, a regression there would silently slip through. Worth one more test asserting the rejected status when the body length differs from the header.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graphql/medium-upload.test.ts` around lines 43 - 74, Add a new test in graphql/medium-upload.test.ts that exercises handleMediumUploadProxy with a declared "Content-Length" that does not match the actual body size: create a session via createMediumUploadSession, send a PUT Request to `/medium-uploads/${session.id}?token=${session.token}` with headers "Content-Type" set and "Content-Length" set to a value different from the bytes supplied (both smaller and/or larger cases if desired), then assert the response indicates rejection (e.g., non-204 status such as 400/413) and verify that disk.getBytes(session.key) was not written; this will catch regressions where declared length is not enforced.models/admin.test.ts (1)
31-42: 💤 Low valueStub disk via the cast escape hatch only catches
delete().
createTrackingDiskusesunknown as Parameters<typeof deleteOrphanMedia>[1]to satisfy the second argument's type. IfdeleteOrphanMediaever starts calling another method on the disk (e.g.,exists,getBytes), this stub will throw at runtime instead of being caught by the type system. Consider either implementing the fullDiskshape or using aPartial<Disk>cast that documents the intent — minor, but worth noting since the cast is the only place the test contract is enforced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/admin.test.ts` around lines 31 - 42, createTrackingDisk currently casts a minimal stub to Parameters<typeof deleteOrphanMedia>[1] using `unknown as ...` which only implements delete() and will break if deleteOrphanMedia calls other Disk methods; replace this unsafe cast by returning either a proper Disk-shaped object that implements all methods deleteOrphanMedia might call (e.g., delete, exists, getBytes, etc.) or explicitly cast to Partial<Disk> (e.g., `as Partial<Disk>`) and implement/document each stubbed method; update createTrackingDisk to expose deleteKeys and implement any additional methods referenced by deleteOrphanMedia so the test remains type-safe and fails at compile time if the contract changes.graphql/schema.graphql (1)
520-521: ⚡ Quick winUse a storage-agnostic name for the new failure counter.
This PR moves media to FlyDrive/object keys, so a brand-new public field named
failedDiskDeletesis already out of date and leaks an implementation detail into the API. A neutral name likefailedStorageDeleteswill age much better.Suggested rename
- """Number of stored media objects that could not be deleted from disk.""" - failedDiskDeletes: Int! + """Number of stored media objects that could not be deleted from storage.""" + failedStorageDeletes: Int!🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graphql/schema.graphql` around lines 520 - 521, The public GraphQL field failedDiskDeletes leaks an implementation detail; rename it to a storage-agnostic failedStorageDeletes across the schema (graphql/schema.graphql) and update all corresponding resolver functions, type definitions, tests, and any client/query usages to the new symbol so the API remains neutral to backend storage changes. Ensure the resolver that previously returned failedDiskDeletes now returns failedStorageDeletes (preserve semantics and type Int!), update any documentation and migration notes, and run tests to catch remaining references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drizzle/0098_unified_medium.sql`:
- Around line 134-149: The INSERT uses DISTINCT ON ("key") without ORDER BY so
PostgreSQL picks an arbitrary row per key; make it deterministic by adding an
ORDER BY that specifies which legacy row to prefer (e.g., choose the most recent
article_medium row). Modify the SELECT to include ORDER BY "key", "created" DESC
(or whichever deterministic tie-breaker you want) so DISTINCT ON("key")
consistently picks the same row and ensures medium.created is stable; this
change should be applied to the INSERT ... SELECT from "article_medium" that
inserts into "medium".
In `@graphql/medium-upload.ts`:
- Around line 100-118: The code currently trusts Content-Length and uses
request.arrayBuffer(), which can let an attacker cause the server to allocate a
large buffer; change the read to a capped streaming read instead: replace the
call to request.arrayBuffer() in the upload handler with a streaming reader
(e.g. request.body.getReader()) that accumulates chunks into a
Uint8Array/temporary buffer but stops and returns a 413 immediately if the
accumulated byte count exceeds Math.min(session.contentLength,
MAX_STREAMING_MEDIUM_IMAGE_SIZE) or differs from session.contentLength when
finished; keep the existing checks against session.contentLength and
MAX_STREAMING_MEDIUM_IMAGE_SIZE, but enforce the cap while reading to avoid
buffering oversized bodies.
In `@graphql/post.ts`:
- Around line 2069-2080: The catch block in the createMediumFromUrl call is
incorrectly mapping all failures to InvalidInputError; change it so only true
validation/formatting failures become InvalidInputError (e.g., when
createMediumFromUrl returns null or throws a known validation error type), but
let storage/network/DB errors bubble up or be converted to a 5xx/internal error
instead. Concretely, update the try/catch in the code around createMediumFromUrl
(and the similar block in finishMediumUpload that interacts with
createMediumFromBytes) to: check the return value (if medium === null) and throw
InvalidInputError("url"); in the catch, inspect the thrown error and rethrow it
unless it is a validation-specific error type, or wrap unexpected errors in an
internal/server error so successful writes are not reported as invalid input.
Ensure you reference createMediumFromUrl, finishMediumUpload, and
createMediumFromBytes when making these changes.
In `@graphql/schema.graphql`:
- Around line 1812-1814: The schema's write-side migration added
UpdateAccountInput.avatarMediumId but the Account output type still only exposes
avatarUrl; update the read side by adding avatarMediumId to the Account type and
mark avatarUrl as deprecated (with a reason pointing to avatarMediumId) so the
new field is discoverable via introspection; also ensure any resolvers for
Account (e.g., account resolver or Account type resolvers) return avatarMediumId
from the underlying model/DB (falling back to avatarUrl if needed) so both
schema and runtime behavior are aligned.
In `@models/admin.ts`:
- Around line 308-323: deleteOrphanMedia currently deletes medium rows first
then calls disk.delete, which makes failed disk deletions unrecoverable; change
the flow to fetch candidate keys (using orphanMediaWhere) first, attempt
disk.delete for each key and collect successes, then delete only the DB rows for
keys successfully removed (or mark failures for retry). Specifically: in
deleteOrphanMedia (use mediumTable, orphanMediaWhere, and disk.delete), query
keys to delete, loop attempting disk.delete and build a successKeys array and
increment failedDiskDeletes for failures (logging warnings), then run
db.delete(mediumTable).where(key in successKeys).returning(...) to remove only
the successfully deleted entries so failed deletes remain retryable.
In `@models/article.ts`:
- Around line 300-310: The substring check using
source.content.includes(`hp-medium:${medium.key}`) can false‑positive on
prefixes; update the logic that builds media (using sourceMedia, source.content,
articleSource.id and inserting into articleSourceMediumTable) to first extract
actual referenced keys from source.content (e.g., run a single global regex
similar to resolveMediumUrls's character class to capture full hp-medium:KEY
tokens or split on token boundaries), build a Set of exact keys, then filter
sourceMedia by membership in that Set so only exact key matches are inserted
(keep the insert into articleSourceMediumTable and .onConflictDoNothing()
unchanged).
In `@models/medium.ts`:
- Around line 135-152: The sharp image decoding/processing block can throw on
corrupt bytes or mismatched Content-Type, so wrap the call that creates and
processes the image (the sharp(...).rotate().webp().toBuffer({...}) sequence
used in createMediumFromBytes()/createMediumFromUrl()) in a try/catch and treat
any thrown error as an invalid medium by returning undefined; keep subsequent
checks (width/height/contentHash) the same and only proceed on success, and do
not rethrow the sharp error so bad uploads fail validation instead of causing a
server error.
- Around line 215-221: The code currently calls response.blob() and only checks
blob.size after the full body is downloaded, which can OOM if Content-Length is
missing/dishonest; instead, read response.body as a stream (use the
ReadableStream reader from response.body) and accumulate bytes up to maxSize,
aborting/canceling the reader and returning undefined as soon as accumulated >
maxSize; replace the blob() call and subsequent blob.size check with this
streaming read so contentLength and maxSize are enforced eagerly (refer to
variables contentLength, maxSize, response, response.body, and the existing
response.blob() usage to locate the change).
In `@models/note.ts`:
- Around line 288-291: The note creation must fail if an attachment can't be
persisted: in createNote() (and the analogous block at lines referenced 327-338)
stop the workflow when createMediumFromBlob/createNoteSourceMedium or the medium
lookup returns null/undefined by aborting note creation immediately (return
undefined or throw) instead of continuing and publishing a note without the
attachment; update the code paths around the medium = "blob" in input ? await
createMediumFromBlob(...) : await db.query.mediumTable.findFirst(...) checks so
that medium == null causes an immediate failure return and does not proceed to
publishing or saving the note.
In `@web-next/src/locales/zh-TW/messages.po`:
- Around line 21-24: The PR introduces the terms "孤立媒體" (orphan media) and
"磁碟物件" (disk object) in the zh-TW locale messages (e.g., the plural msgid string
for failed deletions) but they are missing from the zh-TW glossary; update the
glossary to include entries mapping "orphan media → 孤立媒體" and "disk object →
磁碟物件" (or replace the occurrences in the affected messages with the existing
glossary-approved terms if they already exist), and then ensure the translations
in messages.po (the plural msgid and other affected message strings) use those
glossary terms consistently.
In `@web-next/src/routes/`(root)/admin/media.tsx:
- Around line 146-150: The Show conditional using when={data.viewer?.moderator}
treats both unauthenticated users and authenticated non-moderators the same,
causing signed-in non-moderators to be redirected to sign-in; change the
fallback so it checks data.viewer !== null first and: if viewer is null Navigate
to "/sign?next=%2Fadmin%2Fmedia", otherwise render a forbidden message or
Navigate to "/" (adjust the Show fallback that currently returns <Navigate ...
/>), referencing the Show component, data.viewer and the current fallback
Navigate to locate and update the logic.
In `@web/routes/`@[username]/settings/index.tsx:
- Around line 61-65: The POST handler is missing an ownership/session check
allowing cross-account updates; before applying updates (after obtaining account
via db.query.accountTable.findFirst using ctx.params.username) verify
ctx.state.session exists and that account.id === ctx.state.session.accountId,
and if not return ctx.next() or respond with a 403/permission error, ensuring
the same session-based guard used by the GET path is applied to the POST flow to
prevent unauthorized mutations.
---
Outside diff comments:
In `@federation/objects.ts`:
- Around line 57-64: The mapping over articleSource.contents currently spreads
the original content object after awaiting renderMarkup, so any transformed
source text (resolved from hp-medium: by renderMarkup) gets overwritten and raw
hp-medium: tokens remain in Article.source.content; change the merge order so
the properties returned by await renderMarkup(ctx, content.content, { docId:
articleSource.id, kv: ctx.data.kv, mediumUrls }) take precedence (or explicitly
set the source/content field from the render result) instead of being
overwritten by ...content, and apply the same fix to the similar block
referenced at lines 102-105; locate this in the articleSource.contents map and
ensure the final object uses the rendered content value for source/content.
In `@graphql/schema.graphql`:
- Around line 658-675: The schema removed legacy "*At" fields which breaks
existing clients; add back deprecated alias fields alongside the new names: in
type InvitationRegenerationStatus keep the new fields cutoffDate,
eligibleAccountsCount, lastRegenerated (DateTime), and topThirdCount, and add
deprecated aliases lastRegeneratedAt: DateTime `@deprecated`(reason: "Use
`lastRegenerated`") and regeneratedAt or regenerated? (match the other type's
rename) as deprecated aliases for the corresponding new fields; do the same for
the other occurrence referenced (lines ~1468-1476) so both types expose the new
field names plus the old "*At" fields marked with `@deprecated` and identical
types/descriptions to allow a migration path for clients.
In `@models/post.ts`:
- Around line 349-358: The insert into postMediumTable currently hardcodes type
to "image/webp"; change it to use the stored medium.type from noteSource.media
(e.g., medium.type) when building the object for insertion, and optionally
validate or fallback if medium.type is missing/unsupported (keep
disk.getUrl(medium.medium.key), postId, index, alt, width, height as-is). Update
the mapping that builds the values (the Promise.all over noteSource.media) so
the inserted record's type field reflects Medium.type instead of the literal
"image/webp".
In `@test/postgres.ts`:
- Around line 314-325: The in-memory disk stub created by createTestDisk() has a
delete() method that currently takes no arguments and doesn't remove entries
from the files map; update delete() to accept a key parameter (key: string) and
call files.delete(key) before resolving so it matches the Disk contract used by
disk.delete(key) callers (see getBytes and put for existing behaviors) and
ensures getBytes(key) will throw after deletion.
---
Nitpick comments:
In `@drizzle/0098_unified_medium.sql`:
- Around line 30-58: Add b-tree indexes on the new foreign-key columns to avoid
full-table scans: create an index on note_source_medium("medium_id"), on
article_draft_medium("medium_id"), and on article_source_medium("medium_id"); if
there is a medium_id FK on account, add an index on account("medium_id") as
well. Use clear, unique names (e.g. note_source_medium_medium_id_idx,
article_draft_medium_medium_id_idx, article_source_medium_medium_id_idx,
account_medium_id_idx) and create them alongside the CREATE TABLE statements so
reverse lookups and orphan-media/delete paths use the index.
In `@graphql/medium-upload.test.ts`:
- Around line 43-74: Add a new test in graphql/medium-upload.test.ts that
exercises handleMediumUploadProxy with a declared "Content-Length" that does not
match the actual body size: create a session via createMediumUploadSession, send
a PUT Request to `/medium-uploads/${session.id}?token=${session.token}` with
headers "Content-Type" set and "Content-Length" set to a value different from
the bytes supplied (both smaller and/or larger cases if desired), then assert
the response indicates rejection (e.g., non-204 status such as 400/413) and
verify that disk.getBytes(session.key) was not written; this will catch
regressions where declared length is not enforced.
In `@graphql/schema.graphql`:
- Around line 520-521: The public GraphQL field failedDiskDeletes leaks an
implementation detail; rename it to a storage-agnostic failedStorageDeletes
across the schema (graphql/schema.graphql) and update all corresponding resolver
functions, type definitions, tests, and any client/query usages to the new
symbol so the API remains neutral to backend storage changes. Ensure the
resolver that previously returned failedDiskDeletes now returns
failedStorageDeletes (preserve semantics and type Int!), update any
documentation and migration notes, and run tests to catch remaining references.
In `@models/account.ts`:
- Around line 125-147: The function getAccountByUsername currently queries
avatarMedium but its explicit return type omits that relation; update
getAccountByUsername's declared return type to include the avatarMedium relation
(matching the type returned by db.query.accountTable.findFirst with with: {
avatarMedium: true }) so callers get the correct shape without casts; locate
getAccountByUsername in models/account.ts and extend its return/interface to
include avatarMedium (nullable if DB allows null) or use the generated query
result type that includes with: { avatarMedium: true } for accuracy.
In `@models/admin.test.ts`:
- Around line 31-42: createTrackingDisk currently casts a minimal stub to
Parameters<typeof deleteOrphanMedia>[1] using `unknown as ...` which only
implements delete() and will break if deleteOrphanMedia calls other Disk
methods; replace this unsafe cast by returning either a proper Disk-shaped
object that implements all methods deleteOrphanMedia might call (e.g., delete,
exists, getBytes, etc.) or explicitly cast to Partial<Disk> (e.g., `as
Partial<Disk>`) and implement/document each stubbed method; update
createTrackingDisk to expose deleteKeys and implement any additional methods
referenced by deleteOrphanMedia so the test remains type-safe and fails at
compile time if the contract changes.
In `@web-next/src/locales/zh-CN/messages.po`:
- Around line 1750-1752: The Chinese translation for the msgid in
src/routes/(root)/admin/media.tsx:163 uses an awkward literal "已足够旧"; update the
corresponding msgstr to a more idiomatic phrase such as replacing
"已足够旧且不再附加到头像、帖子、文章草稿或文章的已存储媒体。" with a clearer wording like
"移除已过期且不再附着于头像、帖子、文章草稿或文章的已存储媒体。" — edit the msgstr for that msgid in
messages.po to use "已过期" (or another natural alternative) and keep the rest of
the sentence structure intact.
In `@web-next/src/routes/`(root)/[handle]/settings/index.tsx:
- Around line 283-287: The catch block that currently calls console.error(error)
after createMediumFromDataUrl(pendingAvatarUrl) should use the project's
structured logger LogTape and include the current save context; replace the raw
console.error with a LogTape error call that logs the caught error plus
structured fields such as the save object/identifier and pendingAvatarUrl so the
failure in createMediumFromDataUrl and the avatar upload (avatarMediumId) path
is recorded with proper context.
In `@web/routes/_app.tsx`:
- Around line 73-78: The inline import for Medium should be hoisted to the
existing top-level type imports: add Medium to the existing type import list
that already includes Account and AccountEmail, then update the local variable
type annotation for account (the union type using Account & { emails:
AccountEmail[]; avatarMedium?: import("@hackerspub/models/schema").Medium |
null; }) to reference Medium directly (avatarMedium?: Medium | null). Modify the
top-level type import and the account declaration to use the imported Medium
symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 790d12b6-e76d-4ae7-aa62-0c6a9303241e
⛔ Files ignored due to path filters (2)
web-next/src/routes/(root)/admin/__generated__/mediaDeleteOrphanMediaMutation.graphql.tsis excluded by!**/__generated__/**web-next/src/routes/(root)/admin/__generated__/mediaPageQuery.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (68)
drizzle/0098_unified_medium.sqldrizzle/0099_drop_note_source_medium_unique.sqldrizzle/meta/0098_snapshot.jsondrizzle/meta/0099_snapshot.jsondrizzle/meta/_journal.jsonfederation/actor.tsfederation/objects.tsgraphql/account.test.tsgraphql/account.tsgraphql/admin.test.tsgraphql/admin.tsgraphql/builder.tsgraphql/deno.jsongraphql/main.tsgraphql/medium-upload.test.tsgraphql/medium-upload.tsgraphql/post.more.test.tsgraphql/post.tsgraphql/schema.graphqlgraphql/server.tsgraphql/signup.tsmodels/account.more.test.tsmodels/account.tsmodels/actor.tsmodels/admin.test.tsmodels/admin.tsmodels/article.lifecycle.test.tsmodels/article.tsmodels/markup.tsmodels/medium.test.tsmodels/medium.tsmodels/note.lifecycle.test.tsmodels/note.test.tsmodels/note.tsmodels/post.sync.test.tsmodels/post.tsmodels/relations.tsmodels/schema.tstest/postgres.tsweb-next/relay.config.jsonweb-next/src/components/AppSidebar.tsxweb-next/src/components/article-composer/ArticleComposer.tsxweb-next/src/components/article-composer/ArticleComposerContext.tsxweb-next/src/lib/uploadImage.tsweb-next/src/locales/en-US/messages.poweb-next/src/locales/ja-JP/messages.poweb-next/src/locales/ko-KR/messages.poweb-next/src/locales/zh-CN/messages.poweb-next/src/locales/zh-TW/messages.poweb-next/src/routes/(root)/[handle]/settings/index.tsxweb-next/src/routes/(root)/admin/invitations.tsxweb-next/src/routes/(root)/admin/media.tsxweb/codegen.tsweb/components/AdminNav.tsxweb/main.tsweb/routes/@[username]/[idOrYear]/[slug]/index.tsxweb/routes/@[username]/drafts/[draftId]/publish.tsweb/routes/@[username]/feed.xml.tsweb/routes/@[username]/invite/[id]/index.tsxweb/routes/@[username]/og.tsweb/routes/@[username]/settings/index.tsxweb/routes/_app.tsxweb/routes/admin/index.tsxweb/routes/admin/media.tsxweb/routes/api/media.tsweb/routes/sign/in/[token].tsxweb/routes/sign/up/[token].tsxweb/utils.ts
Bound proxy and remote image reads while streaming, return validation failures for corrupt image bytes, and stop reporting successful uploads as invalid when temporary cleanup fails. hackers-pub#286 (comment) hackers-pub#286 (comment) hackers-pub#286 (comment) hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
Make migrated article media selection deterministic, keep orphan media rows retryable when disk deletion fails, and avoid silent partial success when note or article media references do not match exactly. hackers-pub#286 (comment) hackers-pub#286 (comment) hackers-pub#286 (comment) hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
Expose avatarMediumId on Account, keep signed-in non-moderators out of the sign-in loop, and add glossary terms for the new orphan media cleanup strings. hackers-pub#286 (comment) hackers-pub#286 (comment) hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
Require the route account to match the active session before applying legacy profile settings updates. hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified media management system, replacing the previous note_medium and article_medium tables with a centralized medium table. It includes database migrations to migrate existing data, updates to the GraphQL schema to support the new media structure, and adds functionality for secure media uploads via a proxy. Additionally, it implements a cleanup mechanism for orphan media and updates the web-next stack to handle these changes, including updated internationalization files.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a687bc3989
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Add b-tree indexes for the new medium foreign keys so reverse lookups and orphan media cleanup do not have to scan relation tables. Keep the Drizzle schema and snapshots in sync with the migration. Assisted-by: Codex:gpt-5.5
Deprecate legacy avatar and invitation timestamp aliases while keeping old fields available for clients. Rename the orphan-media deletion failure count to a storage-neutral GraphQL field and update Relay, tests, and extracted translations. Assisted-by: Codex:gpt-5.5
Resolve article medium placeholders in ActivityPub source markdown and copy the stored medium type when syncing note media. Tighten related account and app types so avatar medium relations are visible to callers. Assisted-by: Codex:gpt-5.5
Cover upload proxy rejection when the declared content length is longer than the body. Make the test disk delete keys and reuse the full test disk shape for orphan-media deletion tests. Assisted-by: Codex:gpt-5.5
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 939fb09853
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Keep media referenced directly from article markdown out of orphan cleanup. Process cleanup candidates in bounded batches, delete database rows before storage objects, and cap storage-delete concurrency to avoid unbounded I/O. hackers-pub#286 (comment) hackers-pub#286 (comment) hackers-pub#286 (comment) hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
Return early when a profile settings save is already in progress so duplicate submits cannot start overlapping avatar uploads or account mutations. hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive 'Medium' system to centralize media management, replacing previous scattered implementations (like note_medium and article_medium). It includes database schema updates, a new GraphQL API for media uploads and management, and a migration of existing media data. The reviewer correctly identified a potential issue with the strpos check in the orphan media cleanup logic, which could cause false positives; this feedback is actionable and important for data integrity.
Add an updateArticle media input so edited article markdown can introduce new hp-medium references. The model now validates referenced keys, updates article_source_medium before post rendering, and rolls back the edit when a referenced medium cannot be resolved. Assisted-by: Codex:gpt-5.5
💡 Codex ReviewLines 926 to 928 in 19d1bf7  Reject conflicting draft UUIDs before returning payload
When Lines 349 to 355 in 19d1bf7  Delete storage objects before dropping medium rows
This function deletes ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Match article markdown references as full hp-medium tokens or local media URLs instead of searching for a raw key substring. This keeps direct article references protected without treating prefix-like keys as live. hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
Return InvalidInputError when saveArticleDraft cannot upsert a supplied UUID because it already belongs to another account. This avoids surfacing a non-null payload with a missing draft as an internal GraphQL error. hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
|
Addressed the I am declining the storage-delete ordering suggestion from #286 (comment). The current DB-first order is intentional because it avoids leaving database references pointing at missing objects if a concurrent reference or transaction failure prevents the DB delete. Storage deletion failures are reported, and adding retryable tombstones would be a separate storage cleanup design. |
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request implements a unified "medium" system to centralize media management for avatars, notes, and articles, featuring content-based deduplication and a new upload proxy. The changes span database migrations, GraphQL schema updates, and model-layer logic for both the legacy and new web stacks. Review feedback identifies a memory exhaustion risk in the upload proxy due to full-body buffering, performance concerns regarding redundant regex scans in the orphan media cleanup logic, and an opportunity to optimize the content hashing utility by avoiding unnecessary buffer copies.
I am having trouble creating individual review comments. Click here to see my feedback.
graphql/medium-upload.ts (139-142)
The handleMediumUploadProxy function buffers the entire request body into memory. With MAX_STREAMING_MEDIUM_IMAGE_SIZE set to 50 MiB, concurrent uploads could quickly exhaust server memory. While the current storage abstraction might limit streaming options, consider implementing a more memory-efficient approach if possible, or lowering the limit if 50 MiB is not strictly necessary for all users.
models/admin.ts (270-306)
The regex-based checks on article and draft content are redundant and likely to cause significant performance issues. Since updateArticleSourceMedia ensures that all media referenced in the content have corresponding entries in the article_draft_medium or article_source_medium relation tables, the NOT EXISTS checks on those tables are sufficient to identify orphans. Performing a full-table regex scan on large text columns for every candidate medium is extremely expensive and should be avoided. Removing these checks also allows removing the unused regex pattern definitions.
return sql`\n ${mediumTable.created} < ${cutoffDateSql} AND\n NOT EXISTS (\n SELECT 1 FROM ${accountTable}\n WHERE ${accountTable.avatarMediumId} = ${mediumTable.id}\n ) AND\n NOT EXISTS (\n SELECT 1 FROM ${noteSourceMediumTable}\n WHERE ${noteSourceMediumTable.mediumId} = ${mediumTable.id}\n ) AND\n NOT EXISTS (\n SELECT 1 FROM ${articleDraftMediumTable}\n WHERE ${articleDraftMediumTable.mediumId} = ${mediumTable.id}\n ) AND\n NOT EXISTS (\n SELECT 1 FROM ${articleSourceMediumTable}\n WHERE ${articleSourceMediumTable.mediumId} = ${mediumTable.id}\n )\n `;
models/medium.ts (118-120)
The sha256Hex function creates an unnecessary copy of the input data. crypto.subtle.digest can accept the Uint8Array directly. Passing the Uint8Array instead of copying it into a new buffer improves efficiency, especially for larger media files.
const hashBuffer = await crypto.subtle.digest("SHA-256", data);
💡 Codex ReviewLine 275 in b2c91a4 Match direct media URLs without duplicating media/ prefix
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Preserve direct article references that use the stored medium key path as well as local FS URLs and hp-medium placeholders. This covers CDN-style URLs whose path is based directly on the medium key. hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
Addressed in 07389e3. Orphan cleanup now preserves direct references that use the stored medium key path as well as local /media/ URLs and hp-medium placeholders. |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified 'medium' system to replace fragmented media handling across notes, articles, and user avatars. It adds a new 'medium' table, updates the database schema, and implements a secure proxy for media uploads. The changes include GraphQL schema updates to support the new medium system, migration scripts, and updated test suites. The review comments provided actionable feedback regarding memory management for large file uploads, resource-intensive image processing, concurrency tuning for orphan media deletion, and validation of media key patterns, all of which are valid and constructive.
I am having trouble creating individual review comments. Click here to see my feedback.
graphql/medium-upload.ts (66-91)
The readRequestBody function reads the entire request body into memory. For large uploads (up to 50MB), this could lead to significant memory pressure and potential OOM issues under concurrent load. Consider using a streaming approach if the underlying flydrive disk implementation supports it, or at least ensure that the maxSize is strictly enforced to prevent memory exhaustion.
models/medium.ts (189-192)
Using sharp(input, { animated: true }) is correct for handling GIFs, but be aware that processing large animated images can be CPU and memory intensive. Ensure that the server has sufficient resources to handle concurrent processing of such images.
models/admin.ts (313-331)
The mapWithConcurrency function is a good way to limit concurrency. However, ensure that the ORPHAN_MEDIA_STORAGE_DELETE_CONCURRENCY constant is tuned appropriately for the environment to avoid overwhelming the storage provider or the database.
models/article.ts (54-55)
The regex articleMediumReferencePattern and articleMediumKeyPattern are used for validating media keys. Ensure these patterns are robust enough to handle all valid key formats, as invalid keys could lead to broken article content.
💡 Codex ReviewLines 143 to 146 in 07389e3 Add deterministic ordering for article medium URL maps
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Replace unresolved hp-medium references with generated SVG placeholders when rendering article and draft markup. Use article language where it is available, and keep user-authored data URLs blocked by resolving medium references after Markdown rendering. Cover image, link, and srcset references with regression tests. Assisted-by: Codex:gpt-5.5
Canonicalize medium URL mappings before hashing markup cache keys so unordered database results do not cause cache churn for identical article content. hackers-pub#286 (comment) Assisted-by: Codex:gpt-5.5
|
Addressed #286 (comment) in 52d54da. The markup cache key now canonicalizes medium URL entries before hashing, so unordered DB results do not churn cache keys. |
|
@codex review |
|
/gemini review |
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What changed
This PR replaces the split local media models with a shared
Mediummodel and updates notes, articles, account avatars, GraphQL, legacy web, and web-next to use it.The new
Mediummodel is backed by FlyDrive object keys instead of storing image blobs in the database. It has a UUID primary key, nullable unique SHA-256 content hash, media type, optional image dimensions, and a check constraint that keepswidthandheighteither both null or both present. Local uploads are normalized to WebP.The migration in drizzle/0098_unified_medium.sql moves existing
NoteMedium,ArticleMedium, andAccount.avatarKeydata intoMediumplus the new relation tables. drizzle/0099_drop_note_source_medium_unique.sql relaxes note media attachment uniqueness so the same medium can appear more than once in a note.Model and storage changes
Mediumand the relation tablesNoteSourceMedium,ArticleDraftMedium, andArticleSourceMedium.Account.avatarKeywithAccount.avatarMediumId.NoteSourceMedium, not onMedium.hp-medium:KEY.hp-medium:placeholders in legacy article pages, feeds, ActivityPub objects, drafts, and web-next article rendering.Key files include models/medium.ts, models/schema.ts, models/note.ts, models/article.ts, models/account.ts, graphql/post.ts, graphql/account.ts, graphql/medium-upload.ts, web/routes/admin/media.tsx, and web-next/src/routes/(root)/admin/media.tsx.
GraphQL API changes
This PR adds the
MediumGraphQL type and new upload APIs. All upload mutations require authentication.MediumSimple upload:
createMediumUse this when the client has a small image as a data URL or an HTTP(S) URL. The server fetches the image, validates it, normalizes it to WebP, stores it through FlyDrive, and returns the created
Medium.Example variables:
{ "input": { "url": "data:image/png;base64,..." } }HTTP and HTTPS URLs are also accepted. The server-side fetch path is guarded against SSRF.
Streaming upload:
startMediumUploadandfinishMediumUploadUse this flow for larger images. The GraphQL API creates an upload session and returns a
PUTtarget. The client uploads bytes directly to that URL, then callsfinishMediumUploadto validate and finalize the image as aMedium.Start mutation:
Example variables:
{ "input": { "contentType": "image/png", "contentLength": 1234567 } }Upload step:
Finish mutation:
Example variables:
{ "input": { "uploadId": "019dfc5d-0000-7000-8000-000000000000" } }Attaching media to notes
createNotenow accepts amediaargument. Alt text belongs to the note attachment, not to the sharedMedium.Example variables:
{ "input": { "content": "hello", "language": "en", "visibility": "PUBLIC", "media": [ { "mediumId": "019dfc5d-0000-7000-8000-000000000000", "alt": "Screenshot of the upload dialog" } ] } }Attaching media to article drafts
Article drafts use relation keys so Markdown can refer to images as
hp-medium:KEY.The returned
keycan be used in article Markdown:Attaching media while editing articles
Published articles can also attach new media during
updateArticle. This is the path used when an editor uploads a new image while editing an existing article. The uploadedMediumis passed inUpdateArticleInput.media, and article Markdown refers to it ashp-medium:KEY.Example variables:
{ "input": { "articleId": "QXJ0aWNsZTowMTlkZmM1ZC0wMDAwLTcwMDAtODAwMC0wMDAwMDAwMDAwMDA=", "content": "", "media": [ { "key": "hero", "mediumId": "019dfc5d-0000-7000-8000-000000000000" } ] } }keyis optional and defaults to the medium UUID. The update validates the final Markdown before saving: every newhp-medium:KEYreference must either already be attached to the article source or be present in themediainput. If a referenced medium is missing, the article update fails instead of publishing content with unresolved media placeholders.Account avatars
UpdateAccountInput.avatarUrlis deprecated. New clients should upload a medium first, then passavatarMediumId.avatarUrlandavatarMediumIdare mutually exclusive.Orphan media cleanup
Because we do not have cron-style background jobs yet, this PR adds a moderator-only manual cleanup path.
GraphQL additions:
orphanMediaStatusdeleteOrphanMediaThe cleanup only deletes media older than the grace period and not referenced by accounts, notes, article drafts, or article sources. There are admin UI entry points in web/routes/admin/media.tsx and web-next/src/routes/(root)/admin/media.tsx.
Compatibility notes
uploadMedia()was removed from GraphQL because it could not remain backward compatible.avatarUrl, but new clients should usecreateMediumorstartMediumUpload/finishMediumUploadplusavatarMediumId.hp-medium:references and resolved where rendered or federated.Tests
deno task checkdeno task test