web-next: Add image attachment to note composer#287
Conversation
- Add generateAltText() to the ai/ package with locale-specific system prompts (en, ko, ja, zh-CN, zh-TW); use negotiateLocale() for correct Chinese script fallback (zh-Hant→zh-TW, zh-Hans→zh-CN) - Cap context argument at 1000 characters and set maxOutputTokens: 200 to bound token cost - Export ./alttext from ai/ package (deno.json + package.json) - Add altTextGenerator (gemini-3.1-flash-lite-preview) to graphql/ai.ts and web/ai.ts; upgrade translator to claude-sonnet-4-6 (graphql/ only) - Thread altTextGenerator through ServerContext so tests can inject mocks - Add Medium.generatedAltText(language, context) GraphQL field with complexity 1000 and authentication guard - Add graphql/medium.test.ts covering guest rejection and authenticated success and context-passing cases - Add default no-op altTextGenerator to makeUserContext/makeGuestContext in test/postgres.ts Assisted-by: Claude Code:claude-sonnet-4-6
- Add startMediumUploadOnServer and finishMediumUploadOnServer server
actions to uploadImage.ts, using the startMediumUpload /
finishMediumUpload GraphQL API; finishMediumUpload now returns the
Relay Medium ID needed for alt text generation
- Add uploadMediumWithProgress.ts: client-side XHR upload with progress
callback, timeout (5 min), onabort/ontimeout error handling, and
session.method used from server response
- Redesign NoteComposer to support image attachment:
- Clipboard paste, drag-and-drop, and file-dialog attachment
- Per-image upload progress bar with overlay
- Alt text textarea (required before submission)
- "Auto-fill" button that calls Medium.generatedAltText via Relay
- Max 20 images enforced in UI
- Object URL cleanup on unmount and on failed upload
- Drop handler gated on dataTransfer.types.includes("Files")
- Visibility selector wrapped in role=group for screen-reader label
- Add translations for all 11 new i18n strings across ja-JP, ko-KR,
zh-CN, zh-TW
Assisted-by: Claude Code:claude-sonnet-4-6
- graphql/post.ts: enforce 20-media server-side cap on CreateNoteInput - uploadMediumWithProgress.ts: expose AbortController-based UploadHandle so callers can cancel in-flight uploads; XHR abort is wired to the AbortSignal; UploadAbortedError distinguishes cancellation from failure - NoteComposer.tsx: store abort handle per MediaItem; call abort on image remove, resetForm, and onCleanup; skip error toast on aborted uploads; add aria-label to each alt text textarea using 1-based index Assisted-by: Claude Code:claude-sonnet-4-6
The fallback upload proxy is called cross-origin (web-next at :5173
against the GraphQL server at :8080), so it needs CORS headers.
- Respond to OPTIONS preflight with 204 and appropriate
Access-Control-Allow-{Origin,Methods,Headers} headers
- Reflect the request Origin in Access-Control-Allow-Origin on all
responses (no header added when no Origin is present)
- Add Vary: Origin to prevent incorrect caching
- Add two tests: preflight handling and CORS header on success
Assisted-by: Claude Code:claude-sonnet-4-6
Two Firefox-specific problems prevented the dashed drop zone from
appearing when dragging an image over the note composer:
1. dataTransfer.types is DOMStringList in Firefox, which does not have
Array.prototype.includes. Replaced .includes("Files") with
Array.from(types).includes("Files") for cross-browser compatibility.
2. dragover + dragleave combination flickers when the pointer moves into
a child element (e.g. the textarea): the form receives dragleave
immediately, clearing isDraggingOver before dragover can re-set it.
Replaced with a dragenter/dragleave counter: isDraggingOver stays
true as long as the drag remains anywhere within the form, and only
clears when the counter returns to zero (i.e. the drag has truly left
the form area). dragover is kept solely to call preventDefault (which
is required by the browser to allow the drop).
Assisted-by: Claude Code:claude-sonnet-4-6
Replace the bubble-phase onDragEnter/onDragLeave counter approach with capture-phase addEventListener calls registered in onMount. The root cause was that Firefox's native <textarea> drag handler intercepts dragenter/dragover/drop in the bubble phase before they reach the <form> ancestor, so calling preventDefault() there never suppressed the browser default. Capture-phase listeners on the form fire before any descendant's handler, which lets us call preventDefault() on dragover and drop before Firefox's textarea handler sees the event. The dragCounter is replaced with a relatedTarget check in dragleave: if relatedTarget is still inside the form the drag hasn't actually left, so we keep the dashed outline visible. Assisted-by: Claude Code:claude-sonnet-4-6
Codex review identified that any authenticated user who obtained a
medium ID could trigger AI processing for images they didn't upload.
Fix: when finishMediumUpload creates a medium, store the uploader's
account ID in KV under medium-owner/{mediumId} with a 2-hour TTL
(long enough to compose and post a note). The generatedAltText
resolver checks this KV entry; if it exists and does not match the
caller's account, it throws NotAuthorizedError.
After the TTL expires the gate lifts — the medium is either attached
to a published note (public) or will be removed by orphan-media
cleanup.
Assisted-by: Claude Code:claude-sonnet-4-6
With a single medium-owner/{mediumId} entry, the second user to upload
the same image (content-hash dedup) would overwrite the first user's
ownership marker and block their generatedAltText calls.
Fix: replace the single entry with two separate keys:
- medium-owner/{mediumId}/{accountId} -- per-uploader marker (true)
- medium-upload-window/{mediumId} -- shared window marker (true)
Both use a 2-hour TTL. The generatedAltText resolver first checks the
per-account key (allow if present), then checks the window marker (deny
if the window is active but the per-account key is absent, i.e. a
different account uploaded it recently). If neither key exists the
window has expired and any authenticated user may call the field.
This means multiple uploaders of the same image can independently call
generatedAltText during their respective windows.
Assisted-by: Claude Code:claude-sonnet-4-6
Adds four targeted tests for the KV ownership gate: - owner allowed while upload window is active - non-owner denied while upload window is active - any authenticated user allowed after window expires (no KV entries) - two accounts uploading identical content (dedup) both get access Also stages graphql/schema.graphql, which was updated by Pothos codegen with the revised generatedAltText field description. Assisted-by: Claude Code:claude-sonnet-4-6
|
@codex review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces AI-powered alt text generation for uploaded images. It adds a ChangesAI Alt Text and Medium Upload with Ownership & Auto-Fill UI
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant Server
participant GraphQL
participant KV
participant AI
User->>Browser: Attach image to note
Browser->>Server: POST startMediumUpload
Server->>GraphQL: mutation startMediumUpload
GraphQL->>Server: {uploadUrl, uploadId, headers}
Browser->>Browser: Show upload progress
Browser->>Server: PUT image bytes (XHR)
Server-->>Browser: 204 (with CORS) / progress
Browser->>Server: POST finishMediumUpload(uploadId)
Server->>GraphQL: mutation finishMediumUpload
GraphQL->>KV: setMediumOwner(mediumId, userId)
KV-->>GraphQL: ownership recorded
GraphQL-->>Server: medium created
User->>Browser: Click "Auto-fill alt text"
Browser->>GraphQL: query Medium.generatedAltText(language, context)
GraphQL->>KV: isMediumOwner OR isMediumUploadWindowActive
KV-->>GraphQL: access allowed
GraphQL->>AI: generateAltText(url, language, context)
AI-->>GraphQL: alt text
GraphQL-->>Browser: {generatedAltText}
Browser->>Browser: Populate alt text field
User->>Browser: Submit note with media
Browser->>GraphQL: mutation createNote(media: [{uuid, altText}])
GraphQL->>Server: create note + attachments
Server-->>Browser: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 AI-powered alt text generation and image attachment capabilities to the note composer, featuring multi-language support and ownership-based access control for media. Review feedback suggests optimizing performance by caching static AI prompts in memory and improving security by whitelisting CORS origins instead of echoing request headers. Further recommendations include using Solid.js stores for more efficient state updates of media items and providing explicit user notifications when attachment limits are exceeded.
![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: 9f5704eb8a
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
graphql/medium-upload.ts (1)
175-185:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn CORS headers on the 404/405 exits too.
These are the only proxy responses in this handler that still skip
corsHeaders(request), so a cross-origin client will see them as opaque network errors instead of readable upload failures.Suggested change
if (request.method !== "PUT") { return new Response("Method Not Allowed", { status: 405, + headers: corsHeaders(request), }); } const uploadId = match[1]; if (!validateUuid(uploadId)) { return new Response("Not Found", { status: 404, + headers: corsHeaders(request), }); }🤖 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.ts` around lines 175 - 185, The early-return responses for invalid method and invalid uploadId in the medium upload handler omit CORS headers, causing cross-origin clients to get opaque errors; update the two Response returns (the "Method Not Allowed" 405 branch that checks request.method !== "PUT" and the 404 branch after validateUuid(uploadId)) to include corsHeaders(request) in their Response init headers so they return the same CORS headers as the normal code paths.web-next/src/locales/ja-JP/messages.po (1)
271-379:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the indexed alt-text label to this catalog.
NoteComposer.tsxnow usest\Alt text for image ${index() + 1}`, but this NoteComposer translation block does not addmsgid "Alt text for image {0}"`. Japanese screen readers will fall back to English for that textarea label.Suggested catalog entry
+ #: src/components/NoteComposer.tsx:808 + msgid "Alt text for image {0}" + msgstr "画像{0}の代替テキスト"🤖 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/ja-JP/messages.po` around lines 271 - 379, Add a new catalog entry for the indexed alt-text label used by NoteComposer.tsx: add msgid "Alt text for image {0}" with an appropriate Japanese msgstr (e.g. "画像{0}の代替テキスト" or variant you prefer), preserving the {0} placeholder; this ensures the call t`Alt text for image ${index() + 1}` resolves to a localized string for screen readers and textarea labels.
🧹 Nitpick comments (3)
web-next/src/lib/uploadImage.ts (1)
235-255: ⚡ Quick winInclude structured error context for non-success GraphQL union branches.
Both new upload actions collapseInvalidInputError(and other non-payload responses) into"Upload failed", which loses actionable context. Please log/throw with__typenameandinputPathso failures are diagnosable.Proposed patch
@@ const data = result.data?.startMediumUpload; if (data == null) throw new Error("Upload failed"); @@ if (data.__typename === "NotAuthenticatedError") { throw new Error("Not authenticated"); } - throw new Error("Upload failed"); + throw new Error( + `Upload failed (startMediumUpload: ${data.__typename}${ + data.inputPath ? `, inputPath=${data.inputPath}` : "" + })`, + ); } @@ const data = result.data?.finishMediumUpload; if (data == null) throw new Error("Upload failed"); @@ if (data.__typename === "NotAuthenticatedError") { throw new Error("Not authenticated"); } - throw new Error("Upload failed"); + throw new Error( + `Upload failed (finishMediumUpload: ${data.__typename}${ + data.inputPath ? `, inputPath=${data.inputPath}` : "" + })`, + ); }As per coding guidelines: “Use structured logging via LogTape and include context in error details”.
Also applies to: 303-317
🤖 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/lib/uploadImage.ts` around lines 235 - 255, The current branch handling in the startMediumUpload result (within uploadImage.ts around the StartMediumUploadPayload check and subsequent error branches) collapses non-success GraphQL union variants into a generic "Upload failed"; update the non-payload branches (e.g., when data.__typename !== "StartMediumUploadPayload") to use structured logging via LogTape (or the project's logger) and include the GraphQL __typename and any available inputPath/field info from the response, then throw an Error that contains that same context (e.g., include __typename and inputPath in the message or error metadata) so InvalidInputError and other errors are diagnosable; apply the same change to the other similar block referenced (around lines 303-317) for consistency.web/ai.ts (1)
4-4: LGTM – model identifier is valid.
gemini-3.1-flash-lite-previewwas launched as the first Flash-Lite model in the Gemini 3 series. It's a reasonable cost-efficient choice for high-volume alt-text calls.One heads-up: preview models will typically have billing enabled, might come with more restrictive rate limits, and will be deprecated with at least 2 weeks notice. The team should have a migration plan ready for when this model is eventually promoted to stable or deprecated.
🤖 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/ai.ts` at line 4, The code currently hardcodes the preview model identifier in altTextGenerator (export const altTextGenerator = google("gemini-3.1-flash-lite-preview");) which could cause unexpected billing, rate-limit, or deprecation issues; update the implementation to make the model configurable (e.g., via environment variable or config value) and add a fallback/stable model option and a short TODO comment or migration note near altTextGenerator to ensure the team can swap models quickly when "gemini-3.1-flash-lite-preview" is promoted or deprecated; ensure the symbol altTextGenerator and its producer call to google(...) are the places updated.graphql/medium-upload.test.ts (1)
175-183: ⚡ Quick winConsider asserting
Vary: OriginandAccess-Control-Allow-Headersin the preflight test.The PR description states the proxy "now sends CORS headers (including preflight handling and Vary: Origin)", but neither of the new CORS tests verifies the
Varyheader. A missing or incorrectVary: Originheader can cause CDN/reverse proxy caches to serve the wrong CORS response to subsequent cross-origin requests. Similarly, since the preflight includesAccess-Control-Request-Headers: Content-Type, the response should reflectAccess-Control-Allow-Headers: Content-Type, which is also not checked here.🛡️ Suggested additions to the preflight assertion block
assert.ok( response.headers.get("Access-Control-Allow-Methods")?.includes("PUT"), ); + assert.ok( + response.headers.get("Access-Control-Allow-Headers")?.includes( + "Content-Type", + ), + ); + assert.equal(response.headers.get("Vary"), "Origin");And similarly in the PUT test at line 217:
assert.equal( response.headers.get("Access-Control-Allow-Origin"), "http://localhost:5173", ); + assert.equal(response.headers.get("Vary"), "Origin");🤖 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 175 - 183, The preflight test in graphql/medium-upload.test.ts currently asserts status and some CORS headers but misses verifying the Vary and allowed headers; update the preflight assertion block (the test that checks response.status === 204 and Access-Control-Allow-Methods includes "PUT") to also assert response.headers.get("Vary") includes "Origin" and that response.headers.get("Access-Control-Allow-Headers") includes "Content-Type" (since the request sends Access-Control-Request-Headers: Content-Type). Also add a similar assertion to the PUT test's response checks to ensure response.headers.get("Vary") includes "Origin" so cached responses vary by Origin.
🤖 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 `@graphql/medium-upload.ts`:
- Around line 31-45: The setMediumOwner function must fail closed: after calling
kv.set(getMediumOwnerKey(...), ...) and then
kv.set(getMediumUploadWindowKey(...), ...), if the second set fails you must
roll back the first write by calling kv.delete(getMediumOwnerKey(mediumId,
accountId)) (await it) and then rethrow the original error; ensure both sets are
awaited, handle any delete error (log or swallow) but do not mask the original
failure so callers see the upload-window set error.
---
Outside diff comments:
In `@graphql/medium-upload.ts`:
- Around line 175-185: The early-return responses for invalid method and invalid
uploadId in the medium upload handler omit CORS headers, causing cross-origin
clients to get opaque errors; update the two Response returns (the "Method Not
Allowed" 405 branch that checks request.method !== "PUT" and the 404 branch
after validateUuid(uploadId)) to include corsHeaders(request) in their Response
init headers so they return the same CORS headers as the normal code paths.
In `@web-next/src/locales/ja-JP/messages.po`:
- Around line 271-379: Add a new catalog entry for the indexed alt-text label
used by NoteComposer.tsx: add msgid "Alt text for image {0}" with an appropriate
Japanese msgstr (e.g. "画像{0}の代替テキスト" or variant you prefer), preserving the {0}
placeholder; this ensures the call t`Alt text for image ${index() + 1}` resolves
to a localized string for screen readers and textarea labels.
---
Nitpick comments:
In `@graphql/medium-upload.test.ts`:
- Around line 175-183: The preflight test in graphql/medium-upload.test.ts
currently asserts status and some CORS headers but misses verifying the Vary and
allowed headers; update the preflight assertion block (the test that checks
response.status === 204 and Access-Control-Allow-Methods includes "PUT") to also
assert response.headers.get("Vary") includes "Origin" and that
response.headers.get("Access-Control-Allow-Headers") includes "Content-Type"
(since the request sends Access-Control-Request-Headers: Content-Type). Also add
a similar assertion to the PUT test's response checks to ensure
response.headers.get("Vary") includes "Origin" so cached responses vary by
Origin.
In `@web-next/src/lib/uploadImage.ts`:
- Around line 235-255: The current branch handling in the startMediumUpload
result (within uploadImage.ts around the StartMediumUploadPayload check and
subsequent error branches) collapses non-success GraphQL union variants into a
generic "Upload failed"; update the non-payload branches (e.g., when
data.__typename !== "StartMediumUploadPayload") to use structured logging via
LogTape (or the project's logger) and include the GraphQL __typename and any
available inputPath/field info from the response, then throw an Error that
contains that same context (e.g., include __typename and inputPath in the
message or error metadata) so InvalidInputError and other errors are
diagnosable; apply the same change to the other similar block referenced (around
lines 303-317) for consistency.
In `@web/ai.ts`:
- Line 4: The code currently hardcodes the preview model identifier in
altTextGenerator (export const altTextGenerator =
google("gemini-3.1-flash-lite-preview");) which could cause unexpected billing,
rate-limit, or deprecation issues; update the implementation to make the model
configurable (e.g., via environment variable or config value) and add a
fallback/stable model option and a short TODO comment or migration note near
altTextGenerator to ensure the team can swap models quickly when
"gemini-3.1-flash-lite-preview" is promoted or deprecated; ensure the symbol
altTextGenerator and its producer call to google(...) are the places updated.
🪄 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: 9f8d0eee-a977-40d6-892a-5cf17131fe1e
⛔ Files ignored due to path filters (1)
deno.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
ai/alttext.test.tsai/alttext.tsai/deno.jsonai/mod.tsai/package.jsonai/prompts/alttext/en.mdai/prompts/alttext/ja.mdai/prompts/alttext/ko.mdai/prompts/alttext/zh-CN.mdai/prompts/alttext/zh-TW.mdgraphql/ai.tsgraphql/builder.tsgraphql/main.tsgraphql/medium-upload.test.tsgraphql/medium-upload.tsgraphql/medium.test.tsgraphql/post.tsgraphql/schema.graphqltest/postgres.tsweb-next/src/components/NoteComposer.tsxweb-next/src/lib/uploadImage.tsweb-next/src/lib/uploadMediumWithProgress.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/ai.tsweb/main.ts
getAltTextPrompt was re-reading the prompt file from disk on every generatedAltText call. These files are static, so cache them in a module-level Map keyed by locale baseName. hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
Two NoteComposer fixes from PR review:
- Always call e.preventDefault() on dragover for file drags, regardless
of whether the MAX_MEDIA cap is already reached. Without this, a drop
onto a full composer can fall through to the browser default and
navigate away, discarding the in-progress post text.
- Show a warning toast when addFiles silently drops files because the
remaining capacity is exhausted (e.g. the user selected 5 images but
only 2 slots remained).
Also adds the three new translation strings (Alt text for image {0},
Some images were skipped…, Warning) to ja-JP, ko-KR, zh-CN, zh-TW,
and regenerates the en-US catalog with the updated source locations.
hackers-pub#287 (comment)
hackers-pub#287 (comment)
Assisted-by: Claude Code:claude-sonnet-4-6
If kv.set(ownerKey) succeeded but kv.set(windowKey) then threw, the ownership window would silently never be set, and any non-owner could call generatedAltText immediately. Roll back the per-account key on window-key failure so both writes either succeed or neither takes effect. hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
|
@codex review |
|
/gemini review |
The 405 (wrong method) and 404 (invalid upload ID) branches in handleMediumUploadProxy were returning responses without CORS headers. Cross-origin clients would receive opaque errors for those two cases. The preflight test now also asserts Vary: Origin and Access-Control-Allow-Headers: Content-Type, and the PUT success test asserts Vary: Origin. hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
uploadImage.ts: include __typename (and inputPath for InvalidInputError) in thrown errors so failures are diagnosable in logs without a full GraphQL trace. graphql/ai.ts and web/ai.ts: add a TODO comment near altTextGenerator noting that the preview model string should be made configurable via an env var before the model is promoted or deprecated. hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
There was a problem hiding this comment.
Code Review
This pull request introduces an AI-powered alt text generation feature for media, including backend integration with the AI SDK and a new frontend interface in the web-next stack. The changes include adding a new generateAltText service, updating the GraphQL schema and resolvers to support alt text generation with access control, and implementing a drag-and-drop media upload component in NoteComposer with progress tracking and alt text generation capabilities. The review comments regarding the potential failure of the top-level readdir in alttext.ts, the lack of CORS origin validation in the upload proxy, state management efficiency in NoteComposer, subscription lifecycle management for alt text generation, and the need for more descriptive error handling in the XHR upload utility have been noted as actionable feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d37f73a4f
ℹ️ 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".
Keyv adapters can return false instead of throwing when throwOnErrors
is not enabled. Previously both kv.set calls were awaited without
checking the return value, so a silent false from the adapter would
leave one or both keys unwritten while the function returned normally.
Now both writes are checked: false is treated as a failure. Also
updated the rollback catch to use .catch(() => {}) so a rollback
error cannot mask the original failure.
hackers-pub#287 (comment)
Assisted-by: Claude Code:claude-sonnet-4-6
Three related improvements to the NoteComposer upload flow: - addFiles now creates upload handles before building the MediaItem array, so abortUpload is included in the initial item and only one setMediaItems call is needed per batch instead of N+1. - handleGenerateAlt stores the fetchQuery subscription in the MediaItem as altSubscription. The subscription is unsubscribed when the item is removed, on resetForm, and in onCleanup, preventing stale callbacks from firing after an item has been discarded. - xhrUpload now emits specific error messages for HTTP 413 (file too large) and 415 (unsupported type) rather than the generic "Upload failed with status N". hackers-pub#287 (comment) hackers-pub#287 (comment) hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces AI-powered alternative text generation for image attachments within the note composer. Key changes include the addition of a localized AI alt-text generation module, backend ownership tracking to secure AI generation requests during the initial upload window, and a significant update to the NoteComposer component in the web-next stack to support drag-and-drop image uploads with progress indicators and an Auto-fill alt-text feature. The PR also implements CORS preflight handling for the medium upload proxy and includes comprehensive test coverage for the new AI and GraphQL functionality. I have no feedback to provide.
Adds one test per error status (405, 404, 403, 415, 411, 413) that sends a request with an Origin header and asserts both the expected status code and that Access-Control-Allow-Origin / Vary: Origin are present in the response. A shared assertCors() helper avoids repeating the two header checks in each test. hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
new Intl.Locale(language) throws RangeError for invalid inputs
such as "", "en_US", or other malformed BCP-47 tags, bypassing the
negotiateLocale fallback to English. Wrap the constructor in a
try/catch and fall back to Intl.Locale("en") on error.
hackers-pub#287 (comment)
Assisted-by: Claude Code:claude-sonnet-4-6
Two UX/accessibility improvements to the NoteComposer: - Upload and alt-text generation error toasts now use the Error's message when one is available, so specific reasons (413 too large, 415 unsupported type, auth errors) reach the user instead of being collapsed into a generic string. - Alt-text textarea gains required and aria-required="true" so browsers and assistive tech announce the constraint proactively rather than only after the submit-blocked toast fires. hackers-pub#287 (comment) hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements AI-powered alt text generation for image attachments in the NoteComposer component, featuring multi-language support and upload progress tracking. The implementation includes backend ownership markers and CORS support for cross-origin uploads. Feedback highlights an opportunity to improve frontend performance by using Solid's createStore for managing the media list and identifies a model version inconsistency between the legacy and GraphQL stacks.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ 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". |
The graphql/ai.ts translator was upgraded to claude-sonnet-4-6 earlier in this PR but web/ai.ts was missed and still referenced the older claude-sonnet-4-5-20250929 model. hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an AI-powered alt text generation feature for media uploads. It includes a new generateAltText function, updates the GraphQL schema to expose this functionality, and implements the necessary backend logic for managing upload ownership and CORS preflight requests. The frontend now supports image drag-and-drop, upload progress tracking, and AI-assisted alt text generation in the NoteComposer. I have kept the security-related feedback regarding CORS header validation as it represents a significant improvement opportunity, and the performance-related feedback regarding state management in the NoteComposer as it aligns with best practices for SolidJS.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93f3d4a533
ℹ️ 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".
The hardcoded 5-minute timeout could abort legitimate uploads of large files on slow connections: a 50 MB file at 1 Mbps takes ~6.7 minutes, which exceeds the limit even though the server accepts files that size. User-initiated cancellation is already handled via the AbortController, and network failures are caught by onerror, so the fixed timeout adds no value. hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
During OS-file drags, browsers often fire dragleave with relatedTarget == null even while the cursor is still inside the form, which caused the drop-zone highlight to flicker or disappear mid-drag. Replace the relatedTarget check with a 50 ms debounce: dragleave schedules clearing the overlay after 50 ms, and dragenter (which fires before dragleave per spec) cancels the timer. When the cursor is still inside the form the next dragenter always arrives before the timer fires; only when the cursor genuinely leaves does the timer run to completion. hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an AI-powered alt text generation feature for media uploads, including the necessary backend infrastructure, GraphQL schema updates, and a new UI component in the web-next stack. The review identified critical performance issues regarding signal updates in the NoteComposer component and provided security feedback on CORS origin validation. The review comments pointing out these technical issues and improvement opportunities have been retained, while general validation comments were filtered out.
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
Solid's <For> tracks items by reference identity. The previous createSignal approach replaced every updated item with a new object (via spread-copy), causing <For> to destroy and recreate the DOM node for that item on every change. In the alt-text textarea this meant focus and cursor position were lost after every keystroke; for the upload-progress overlay it caused visual flicker on every XHR progress event. Migrating to createStore gives fine-grained reactivity: property mutations on a store item do not change the object's reference, so <For> keeps the existing DOM node alive. All setMediaItems calls are updated to use produce() for in-place mutations, and all reads drop the () call-syntax that signals require. hackers-pub#287 (comment) hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces AI-powered alt text generation for images in the new SolidStart stack. It adds a new generateAltText utility, integrates it into the GraphQL API via a new generatedAltText field on the Medium type, and updates the NoteComposer component to support image uploads with AI-assisted alt text generation. The changes also include CORS handling for image uploads and comprehensive test coverage. I have kept the review comments regarding the potential NaN in progress calculation, the top-level await pattern, and the race condition in addFiles as they provide actionable technical improvements.
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
When lengthComputable is true but e.total is zero, dividing e.loaded by e.total produces NaN. The spec does not explicitly prohibit total === 0 when lengthComputable is true, so add an explicit guard. hackers-pub#287 (comment) Assisted-by: Claude Code:claude-sonnet-4-6
|
@codex review |
|
/gemini review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
There was a problem hiding this comment.
Code Review
This pull request introduces AI-powered alt text generation for image attachments, significantly enhancing accessibility. Key updates include the implementation of the generateAltText utility, backend support for media ownership tracking and CORS-compliant uploads, and a major enhancement to the NoteComposer in the web-next stack to support image attachments with drag-and-drop and AI auto-fill capabilities. Feedback identifies a high-severity issue in the generatedAltText resolver where incorrect type casting of the language argument could lead to runtime errors, suggesting the explicit instantiation of Intl.Locale instead.
1 participant
This PR adds image attachments to the web-next note composer. Uploads report XHR progress, images require alt text before posting, and the composer can request an AI alt-text draft.
The composer accepts images from the file picker, pasted clipboard data, and drag-and-drop. The 20-image cap is enforced in both the UI and the
createNotemutation. The submit button stays disabled while any upload is in-progress.Images go through the existing
/medium-uploads/*proxy in the GraphQL server, which validates content-type and content-length before writing to storage. The proxy now sends CORS headers so the web-next dev server can call it from a different origin.Alt text is required for every attached image. Each one shows a textarea and an “Auto-fill” button that calls a new GraphQL field,
Medium.generatedAltText(language, context), backed bygemini-3.1-flash-lite-preview. The resolver passes the current note body as context for the alt-text prompt.ai/alttext.ts adds
generateAltText(), which wraps the Vercel AI SDK'sgenerateText, loads a locale-specific prompt from ai/prompts/alttext/*.md, and caps output at 200 tokens. Locale matching uses the existingnegotiateLocale()helper, including Traditional/Simplified Chinese fallback.graphql/post.ts adds the
Medium.generatedAltTextfield (complexity 1000) and records the uploader in KV whenfinishMediumUploadsucceeds. The ownership model uses two keys:medium-owner/{mediumId}/{accountId}(per-uploader, 2-hour TTL) andmedium-upload-window/{mediumId}(shared window marker, same TTL). During the window, only uploaders with a per-account key can call the field; after it expires, any authenticated user can. Per-account keys are used specifically because content-hash deduplication means two users can upload the same image and receive the samemediumTablerow—a single owner key would let the second upload overwrite the first user's access.The translator model in graphql/ai.ts is upgraded to
claude-sonnet-4-6.web-next/src/lib/uploadImage.ts gains two server actions:
startMediumUploadOnServerandfinishMediumUploadOnServer. web-next/src/lib/uploadMediumWithProgress.ts orchestrates the three-step flow (start session → XHR PUT with progress events → finish) and exposes anUploadHandlewithabort()so in-flight uploads can be cancelled when an image is removed or the form is reset.web-next/src/components/NoteComposer.tsx now owns the image attachment state: local preview URL, upload progress, alt text, and Relay ID after upload completes. Drag-and-drop uses capture-phase
addEventListenerhandlers instead of Solid JSX event props. Firefox runs the native textarea drag handler in the bubble phase, which consumed the drop event before the form saw it.Translation strings for the new UI are in all five locale files (
en-US,ja-JP,ko-KR,zh-CN,zh-TW).Tests were added in ai/alttext.test.ts, graphql/medium.test.ts, and graphql/medium-upload.test.ts. They cover mocked alt-text generation, the
generatedAltTextGraphQL field, the upload proxy, owner checks during the upload window, post-expiry fallback, and dual-owner deduplication.