feat(#5): Event images — poster library + custom upload#41
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds CLI support for Partiful event images: new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (partiful)
participant PosterCDN as Poster CDN (assets.getpartiful.com)
participant UploadSvc as Upload Service (/uploadPhoto)
participant Firestore as Firestore API
CLI->>PosterCDN: GET /posters.json (fetchCatalog)
Note right of PosterCDN: response cached by fetchCatalog()
CLI-->>CLI: searchPosters / filter / select poster (score)
CLI->>Firestore: POST /createEvent { event.image: buildPosterImage(poster) }
alt custom image upload
CLI->>UploadSvc: POST /uploadPhoto (multipart form with image)
UploadSvc-->>CLI: 200 { uploadData / url / dimensions }
CLI-->>CLI: buildUploadImage(uploadData)
CLI->>Firestore: POST /createEvent { event.image: buildUploadImage(...) }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
tests/posters-integration.test.js (1)
67-71: Avoid hard-coding a single catalog ID in integration tests.Using a fixed poster (
piscesairbrush.png) can fail when catalog content changes. Prefer getting one ID fromposters list --limit 1and using it in theposters getassertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/posters-integration.test.js` around lines 67 - 71, Replace the hard-coded ID in the "returns full poster by ID" test by first calling run(['posters','list','--limit','1']) to retrieve a current catalog entry, extract its id (e.g., from the first item in the list response), then call run(['posters','get', id]) and assert the returned res.data.id equals that extracted id; update the test that currently uses run([... 'piscesairbrush.png']) to use the dynamically obtained id and keep the same success status assertion.tests/events-integration.test.js (1)
129-151: Rename one duplicateJSON envelope shapesuite for readability.There are two top-level
describe('JSON envelope shape', ...)blocks; renaming one would make test output clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/events-integration.test.js` around lines 129 - 151, Two top-level describe blocks share the same title 'JSON envelope shape', so rename one to a more specific name to improve test output clarity; locate the describe block that contains the tests for 'events update --poster in dry-run' and 'events update --image validates extension' and change its title to something like "JSON envelope shape - events update" (or similar specific label) so it is unique while leaving the inner it() test names unchanged.src/commands/posters.js (1)
52-53: PreferEXITconstants over numeric literals for exit codes.Using named constants here would keep error handling consistent and easier to maintain.
Also applies to: 71-72, 83-84, 88-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/posters.js` around lines 52 - 53, Replace hard-coded numeric exit codes passed to jsonError in posters.js with named EXIT constants: import or reference the existing EXIT enum/object and swap uses of numeric literals (e.g., the 5 in jsonError(err.message, 5, 'internal_error') and the other literals at the locations you noted) for the appropriate named constants (for example EXIT.INTERNAL_ERROR or the matching constant name used in the codebase). Update every call to jsonError in this file (including the calls around the jsonError invocation at function scope where you saw lines 52-53, 71-72, 83-84, 88-89) to pass the corresponding EXIT constant instead of the numeric literal and ensure the import/name matches the module that defines EXIT.docs/plans/2026-03-24-event-images-implementation.md (2)
70-82: Add language specifier to code block.The upload endpoint code block lacks a language specifier.
📝 Proposed fix
### Upload Endpoint -``` +```text POST https://api.partiful.com/uploadPhoto Content-Type: multipart/form-data FormData: - file: <binary> - (params are URL-encoded in the callable function URL) Callable function pattern: POST to `${baseUrl}/uploadPhoto` with params: { uploadType: "event_poster" } Returns: { uploadData: { path, url, contentType, size, width, height } }</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/plans/2026-03-24-event-images-implementation.mdaround lines 70 - 82,
The "Upload Endpoint" example code block is missing a language specifier; update
the triple-backtick fence for the POST example under the "Upload Endpoint"
section to include a language tag (e.g., "text" or "http") so the block becomes
text (orhttp) to ensure proper syntax highlighting and consistency with
other examples; modify the fenced block containing the POST
https://api.partiful.com/uploadPhoto example accordingly.</details> --- `19-27`: **Add language specifier to code block.** The code block listing image source types lacks a language specifier. Per markdownlint MD040, fenced code blocks should specify a language for syntax highlighting and accessibility. <details> <summary>📝 Proposed fix</summary> ```diff ### Image Source Types (from app bundle module 90126) -``` +```text GIPHY = "giphy" — GIF search (Giphy API, skip for v1) LOCAL = "local" — client-only, not persisted UNSPLASH = "unsplash" — stock photos (skip for v1) UPLOAD = "upload" — custom image upload PARTIFUL_POSTERS = "partiful_posters" — built-in poster library ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/plans/2026-03-24-event-images-implementation.mdaround lines 19 - 27,
The fenced code block listing image source constants (GIPHY, LOCAL, UNSPLASH,
UPLOAD, PARTIFUL_POSTERS) should include a language specifier to satisfy
markdownlint MD040; update the opening fence fromtotext (or another
appropriate language) so the block becomestext ...to enable proper
syntax highlighting and accessibility.</details> </blockquote></details> <details> <summary>src/lib/upload.js (1)</summary><blockquote> `26-29`: **Blob created without MIME type may cause server-side content detection issues.** The `Blob` is created without specifying the MIME type, which means the multipart body's `Content-Type` header for the file part may be `application/octet-stream` instead of the actual image type. While many servers handle this gracefully, explicitly setting the type ensures consistent behavior. <details> <summary>♻️ Proposed fix to include MIME type</summary> ```diff +const MIME_TYPES = { + '.png': 'image/png', + '.jpg': 'image/jpeg', + '.jpeg': 'image/jpeg', + '.gif': 'image/gif', + '.webp': 'image/webp', + '.avif': 'image/avif', +}; + // In uploadEventImage, after ext validation: - const fileData = readFileSync(filePath); - const blob = new Blob([fileData]); + const fileData = readFileSync(filePath); + const contentType = MIME_TYPES[ext]; + const blob = new Blob([fileData], { type: contentType }); ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/lib/upload.js` around lines 26 - 29, The Blob is created without a MIME type which can make the multipart file part default to application/octet-stream; update the upload logic to detect the file's MIME type (e.g., using a mime utility with filePath or its extension) and pass it into the Blob constructor so formData.append('file', blob, basename(filePath)) sends the correct Content-Type for fileData; locate the Blob creation and formData.append usage in this module and ensure the detected MIME type is provided as the Blob's { type: mimeType } option. ``` </details> </blockquote></details> <details> <summary>src/commands/events.js (1)</summary><blockquote> `33-40`: **Array number handling inconsistent with top-level number handling.** Float values inside arrays will be incorrectly serialized as `integerValue` (line 36), whereas top-level floats correctly use `doubleValue` (lines 28-31). For the current image use case with integer dimensions, this works, but the inconsistency could cause subtle bugs if floats are ever added to array fields. <details> <summary>♻️ Proposed fix for consistent float handling</summary> ```diff fields[key] = { arrayValue: { values: value.map(v => { if (typeof v === 'string') return { stringValue: v }; - if (typeof v === 'number') return { integerValue: String(v) }; + if (typeof v === 'number') { + if (Number.isInteger(v)) return { integerValue: String(v) }; + return { doubleValue: v }; + } if (typeof v === 'object') return { mapValue: { fields: toFirestoreMap(v) } }; return { stringValue: String(v) }; })}}; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/commands/events.js` around lines 33 - 40, The array branch currently serializes all numbers as integerValue; update the mapper inside the Array.isArray(value) branch so numbers are handled the same way as top-level numbers: use Number.isInteger(v) to emit integerValue for integers and doubleValue for non-integer floats, preserving the same string conversion/format used by the top-level serializer; modify the arrow function mapping (the v => { ... } block that calls toFirestoreMap(v) for objects) to include this integer-vs-float check so array elements mirror the behavior of the top-level numeric handling. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@docs/research/2026-03-24-event-image-schema.md:
- Around line 99-101: Update the paragraph that begins "Posters are likely
fetched from Firestore directly (the web app uses Firebase)..." to explicitly
mark it as speculative/hypothesis rather than an implementation instruction:
prepend or wrap it with a clear label such as "Hypothesis / Non-contract" and
change imperative wording (e.g., "we could query") to conditional phrasing
(e.g., "if true, this could be queried"), and add a short note pointing readers
back to the implemented public catalog endpoint approach for the canonical
integration path so future implementers aren’t misled.In
@src/commands/events.js:
- Around line 252-259: searchPosters returns an array of wrappers { poster,
score } but the code calls buildPosterImage with the wrapper, causing malformed
image data; change the use in the opts.posterSearch branch to extract the poster
from the first result (e.g., const poster = results[0].poster) and pass that
poster into buildPosterImage when setting event.image, keeping the surrounding
flow with fetchCatalog, jsonError, and opts.posterSearch intact.- Around line 360-378: The image extension and filename extraction in the events
command are fragile and inconsistent: replace the split-based logic in the
opts.image block (where ext is computed via opts.image.split('.').pop() and
filename via opts.image.split('/').pop()) with Node's path utilities—use
path.extname(opts.image) (and handle missing/empty ext by treating as
unsupported) to compute the extension and path.basename(opts.image) to get the
filename; import/require the path module at top of file, adjust the allowed
check to compare normalized ext (strip leading dot and toLowerCase), and pass
the basename result into buildUploadImage (the same place uploadEventImage and
buildUploadImage are used) so behavior matches create command and works on
Windows and edge cases.In
@src/commands/posters.js:
- Around line 31-32: Validate the --limit option in posters.js inside the
command action before any slicing: ensure opts.limit is parsed as an integer > 0
(and not negative or zero) and if invalid immediately throw or return a
commander-friendly error so execution stops (do this at the top of the .action
handler where opts.limit is used), then use the validated integer for all
subsequent slices; apply the same check in the other action handlers referenced
(lines 48-50, 59-60, 64-70) where opts.limit is consumed.In
@src/commands/schema.js:
- Line 32: Update the CLI schema entry for '--image' to reflect "file path or
URL" and add URL-handling in the command logic: change the '--image' description
to "Custom image file path or URL to upload", then in the command handler that
currently calls uploadEventImage(filePath, ...) detect URLs (e.g., startsWith
http:// or https://) and implement a new helper (e.g., uploadEventImageFromUrl
or extend uploadEventImage) that fetches the remote content, writes to a temp
buffer/path or streams it, and passes the data to the existing upload flow;
ensure the new helper reuses existing upload logic and preserves error
handling/logging.In
@src/lib/posters.js:
- Around line 5-13: fetchCatalog currently can hang indefinitely; wrap the
network call in a timeout using AbortController: create an AbortController,
start a timer (e.g., 5-10s) that calls controller.abort(), pass
controller.signal to fetch('https://assets.getpartiful.com/posters.json', {
signal }), and clear the timer after fetch completes; if abort triggers, throw a
descriptive error (e.g., "Poster catalog fetch timed out") and ensure
_catalogCache is only set when fetch/res.json succeed (do not cache on timeout
or network errors). Use the existing _catalogCache and fetchCatalog identifiers
to locate and modify the code.In
@tests/events-integration.test.js:
- Around line 46-87: The tests in tests/events-integration.test.js (the three
"events create" it blocks and the similar cases at 130-139) rely on the live
poster catalog; replace those live-catalog couplings with deterministic
fixtures/mocks by stubbing the catalog search and poster-fetch endpoints used by
the CLI helpers run()/runRaw() so they return a fixed poster object (e.g., a
known id, source 'partiful_posters', url, and poster metadata) for the --poster
and --poster-search flows; update the tests to use the fixture values (assert
against the fixture id/url/source) and ensure the error case returns a
controlled not_found response from the mocked catalog rather than hitting the
real service.
Nitpick comments:
In@docs/plans/2026-03-24-event-images-implementation.md:
- Around line 70-82: The "Upload Endpoint" example code block is missing a
language specifier; update the triple-backtick fence for the POST example under
the "Upload Endpoint" section to include a language tag (e.g., "text" or "http")
so the block becomestext (orhttp) to ensure proper syntax highlighting
and consistency with other examples; modify the fenced block containing the POST
https://api.partiful.com/uploadPhoto example accordingly.- Around line 19-27: The fenced code block listing image source constants
(GIPHY, LOCAL, UNSPLASH, UPLOAD, PARTIFUL_POSTERS) should include a language
specifier to satisfy markdownlint MD040; update the opening fence fromtotext (or another appropriate language) so the block becomestext ...
to enable proper syntax highlighting and accessibility.In
@src/commands/events.js:
- Around line 33-40: The array branch currently serializes all numbers as
integerValue; update the mapper inside the Array.isArray(value) branch so
numbers are handled the same way as top-level numbers: use Number.isInteger(v)
to emit integerValue for integers and doubleValue for non-integer floats,
preserving the same string conversion/format used by the top-level serializer;
modify the arrow function mapping (the v => { ... } block that calls
toFirestoreMap(v) for objects) to include this integer-vs-float check so array
elements mirror the behavior of the top-level numeric handling.In
@src/commands/posters.js:
- Around line 52-53: Replace hard-coded numeric exit codes passed to jsonError
in posters.js with named EXIT constants: import or reference the existing EXIT
enum/object and swap uses of numeric literals (e.g., the 5 in
jsonError(err.message, 5, 'internal_error') and the other literals at the
locations you noted) for the appropriate named constants (for example
EXIT.INTERNAL_ERROR or the matching constant name used in the codebase). Update
every call to jsonError in this file (including the calls around the jsonError
invocation at function scope where you saw lines 52-53, 71-72, 83-84, 88-89) to
pass the corresponding EXIT constant instead of the numeric literal and ensure
the import/name matches the module that defines EXIT.In
@src/lib/upload.js:
- Around line 26-29: The Blob is created without a MIME type which can make the
multipart file part default to application/octet-stream; update the upload logic
to detect the file's MIME type (e.g., using a mime utility with filePath or its
extension) and pass it into the Blob constructor so formData.append('file',
blob, basename(filePath)) sends the correct Content-Type for fileData; locate
the Blob creation and formData.append usage in this module and ensure the
detected MIME type is provided as the Blob's { type: mimeType } option.In
@tests/events-integration.test.js:
- Around line 129-151: Two top-level describe blocks share the same title 'JSON
envelope shape', so rename one to a more specific name to improve test output
clarity; locate the describe block that contains the tests for 'events update
--poster in dry-run' and 'events update --image validates extension' and change
its title to something like "JSON envelope shape - events update" (or similar
specific label) so it is unique while leaving the inner it() test names
unchanged.In
@tests/posters-integration.test.js:
- Around line 67-71: Replace the hard-coded ID in the "returns full poster by
ID" test by first calling run(['posters','list','--limit','1']) to retrieve a
current catalog entry, extract its id (e.g., from the first item in the list
response), then call run(['posters','get', id]) and assert the returned
res.data.id equals that extracted id; update the test that currently uses
run([... 'piscesairbrush.png']) to use the dynamically obtained id and keep the
same success status assertion.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `2b435162-cbab-4733-ae53-6bdc599f6bba` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c46a4e025574a27ca0cb8628386cbd352de43e26 and 66618ea3c31b50067f8c3f5b72baa215f66b96d9. </details> <details> <summary>📒 Files selected for processing (11)</summary> * `docs/plans/2026-03-24-event-images-implementation.md` * `docs/research/2026-03-24-event-image-schema.md` * `src/cli.js` * `src/commands/events.js` * `src/commands/posters.js` * `src/commands/schema.js` * `src/lib/posters.js` * `src/lib/upload.js` * `tests/events-integration.test.js` * `tests/posters-integration.test.js` * `tests/upload.test.js` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/posters.js (1)
83-98: Consider adding computedthumbnailto thegetresponse for API consistency.The
listandsearchcommands return summarized posters with a computedthumbnailfield, butgetreturns the raw catalog entry without it. This creates a minor inconsistency in the API surface — consumers expectingthumbnailfrom list/search won't find it ingetoutput.If "full poster details" intentionally means raw catalog data, this is fine as-is. Otherwise, consider enriching the response:
♻️ Optional: Add thumbnail to get response
if (!poster) { jsonError(`Poster not found: ${posterId}`, 4, 'not_found'); return; } - jsonOutput(poster); + jsonOutput({ ...poster, thumbnail: posterThumbnail(poster.id) }); } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/posters.js` around lines 83 - 98, The get action returns the raw catalog entry but should mirror the summarized responses by adding the computed thumbnail field: inside the posters.command('get <posterId>').action async handler, after locating poster via fetchCatalog() and before calling jsonOutput(poster), compute and attach the same thumbnail value used by list/search (reuse the thumbnail generation logic or helper used there) so jsonOutput receives poster with poster.thumbnail populated; keep error handling via jsonError unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/upload.js`:
- Around line 56-57: The upload handler currently returns result.uploadData (or
fallbacks) without validating shape, which can make event.image receive objects
missing url; update the code that processes response.json() (the result variable
and expressions result.uploadData and result.result?.uploadData) to validate the
uploadData object has a valid string url (and any other required fields) before
returning it, and if validation fails either throw a descriptive Error or return
a consistent normalized object (or null) so callers assigning to event.image
will not get a malformed payload; apply the same validation to the other upload
return paths in the same file (the block around the other response handling).
- Around line 46-50: The POST upload call (response = await fetch(url, { method:
'POST', headers: { Authorization: `Bearer ${token}` }, body: formData })) has no
timeout and can hang; wrap the request with an AbortController: create an
AbortController, pass controller.signal to fetch, start a timer (e.g. 30s) that
calls controller.abort() on expiry, clear the timer after fetch completes, and
ensure you handle the abort error (check for AbortError) and surface a clear
timeout error from the upload function so callers don't hang indefinitely.
---
Nitpick comments:
In `@src/commands/posters.js`:
- Around line 83-98: The get action returns the raw catalog entry but should
mirror the summarized responses by adding the computed thumbnail field: inside
the posters.command('get <posterId>').action async handler, after locating
poster via fetchCatalog() and before calling jsonOutput(poster), compute and
attach the same thumbnail value used by list/search (reuse the thumbnail
generation logic or helper used there) so jsonOutput receives poster with
poster.thumbnail populated; keep error handling via jsonError unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 174e5ec2-3b12-4aa9-84e7-bcff04e53daa
📒 Files selected for processing (6)
docs/plans/2026-03-24-event-images-implementation.mdsrc/commands/events.jssrc/commands/posters.jssrc/lib/posters.jssrc/lib/upload.jstests/events-integration.test.js
✅ Files skipped from review due to trivial changes (2)
- src/lib/posters.js
- docs/plans/2026-03-24-event-images-implementation.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/events-integration.test.js
- src/commands/events.js
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/commands/events.js (1)
257-265:⚠️ Potential issue | 🔴 CriticalNormalize
searchPosters()results before callingbuildPosterImage().
src/lib/posters.jsreturns flattened matches with an extrascorefield. Line 264 passes that scored object straight intobuildPosterImage(), soscoregets persisted intoevent.image.poster. Line 371 then expectsresults[0].poster, which does not exist, soevents update --poster-searchwill throw.Suggested fix
const results = searchPosters(catalog, opts.posterSearch); if (results.length === 0) { jsonError(`No posters found matching "${opts.posterSearch}". Try "partiful posters search <term>".`, 4, 'not_found'); return; } - event.image = buildPosterImage(results[0]); + const matchedPoster = { ...results[0] }; + delete matchedPoster.score; + event.image = buildPosterImage(matchedPoster); ... const results = searchPosters(catalog, opts.posterSearch); if (results.length === 0) { jsonError(`No posters found matching "${opts.posterSearch}".`, 4, 'not_found'); return; } - poster = results[0].poster; + poster = { ...results[0] }; + delete poster.score;Also applies to: 366-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/events.js` around lines 257 - 265, searchPosters() returns flattened objects with a score field, but buildPosterImage() and later code expect a normalized poster object (e.g. results[0].poster); update the poster-search branch so that after calling searchPosters(catalog, opts.posterSearch) you map/normalize the first match into the expected shape (strip the score and ensure a .poster property or pass results[0].poster) before calling buildPosterImage() and assigning event.image.poster; adjust the same normalization pattern used around the other poster-search handling (lines ~366-375) so buildPosterImage() and subsequent code receive a clean poster object.src/lib/upload.js (2)
57-58:⚠️ Potential issue | 🟠 MajorFail fast on malformed upload responses.
Line 58 can return any JSON envelope, but
buildUploadImage()later assumesuploadData.urlexists. If the upstream response shape drifts, the CLI will emit an invalid image payload instead of surfacing a clear failure.Suggested fix
- const result = await response.json(); - return result.uploadData || result.result?.uploadData || result; + let result; + try { + result = await response.json(); + } catch { + throw new Error('Upload failed: invalid JSON response'); + } + + const uploadData = result?.uploadData ?? result?.result?.uploadData ?? result; + if (!uploadData || typeof uploadData.url !== 'string' || !uploadData.url) { + throw new Error('Upload failed: missing uploadData.url in response'); + } + + return uploadData;Also applies to: 109-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/upload.js` around lines 57 - 58, The function that parses the upload response currently returns any JSON envelope (result variable) but downstream buildUploadImage expects result.uploadData.url; update the parsing here to validate the shape and fail fast: after parsing response.json(), ensure uploadData exists and uploadData.url is a non-empty string and if not throw a clear Error (e.g., "Malformed upload response: missing uploadData.url") so callers don't proceed with invalid payloads; apply the same validation/throw behavior to the other response-parsing block referenced (lines ~109-119) so both places validate before returning.
47-51:⚠️ Potential issue | 🟠 MajorBound the upload request with a timeout.
fetch()on Line 47 is still unbounded, so a stalled upload can hangevents create/events updateindefinitely.Suggested fix
- const response = await fetch(url, { - method: 'POST', - headers: { Authorization: `Bearer ${token}` }, - body: formData, - }); + const timeoutMs = Number(config?.uploadTimeoutMs ?? 30000); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), timeoutMs); + let response; + try { + response = await fetch(url, { + method: 'POST', + headers: { Authorization: `Bearer ${token}` }, + body: formData, + signal: controller.signal, + }); + } catch (err) { + if (err.name === 'AbortError') { + throw new Error(`Upload failed: request timed out after ${timeoutMs}ms`); + } + throw err; + } finally { + clearTimeout(timeout); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/upload.js` around lines 47 - 51, The upload fetch call is unbounded and can hang; modify the upload logic around fetch(url, { method:'POST', headers:{ Authorization:`Bearer ${token}` }, body: formData }) to use an AbortController with a timeout (e.g., UPLOAD_TIMEOUT_MS constant or config), pass controller.signal to fetch, and clear the timer on success; on timeout abort(), reject/throw a clear timeout error so callers of the upload function (where response is awaited) can handle the failure. Ensure you clean up the timeout and surface a descriptive error message when the request is aborted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/upload.js`:
- Around line 90-99: In downloadToTemp(), avoid buffering arbitrary large
responses and hardcoded /tmp: first read response.headers.get('content-length')
and if present and > 10 * 1024 * 1024 throw an error; then when creating the
Buffer from await response.arrayBuffer() re-check buffer.byteLength and throw if
> 10MB as a fallback; replace the hardcoded tempPath construction with
os.tmpdir() + path.join (use path.join(os.tmpdir(),
`partiful-upload-${rand}${ext}`)) for cross-platform temp paths; keep existing
contentType/COLENT_TYPE_TO_EXT validation and ensure callers like
uploadEventImage still enforce the 10MB limit on the final file.
---
Duplicate comments:
In `@src/commands/events.js`:
- Around line 257-265: searchPosters() returns flattened objects with a score
field, but buildPosterImage() and later code expect a normalized poster object
(e.g. results[0].poster); update the poster-search branch so that after calling
searchPosters(catalog, opts.posterSearch) you map/normalize the first match into
the expected shape (strip the score and ensure a .poster property or pass
results[0].poster) before calling buildPosterImage() and assigning
event.image.poster; adjust the same normalization pattern used around the other
poster-search handling (lines ~366-375) so buildPosterImage() and subsequent
code receive a clean poster object.
In `@src/lib/upload.js`:
- Around line 57-58: The function that parses the upload response currently
returns any JSON envelope (result variable) but downstream buildUploadImage
expects result.uploadData.url; update the parsing here to validate the shape and
fail fast: after parsing response.json(), ensure uploadData exists and
uploadData.url is a non-empty string and if not throw a clear Error (e.g.,
"Malformed upload response: missing uploadData.url") so callers don't proceed
with invalid payloads; apply the same validation/throw behavior to the other
response-parsing block referenced (lines ~109-119) so both places validate
before returning.
- Around line 47-51: The upload fetch call is unbounded and can hang; modify the
upload logic around fetch(url, { method:'POST', headers:{ Authorization:`Bearer
${token}` }, body: formData }) to use an AbortController with a timeout (e.g.,
UPLOAD_TIMEOUT_MS constant or config), pass controller.signal to fetch, and
clear the timer on success; on timeout abort(), reject/throw a clear timeout
error so callers of the upload function (where response is awaited) can handle
the failure. Ensure you clean up the timeout and surface a descriptive error
message when the request is aborted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed616e20-23ba-465c-a262-2a7915a3f84e
📒 Files selected for processing (7)
src/commands/events.jssrc/commands/schema.jssrc/lib/posters.jssrc/lib/upload.jstests/events-integration.test.jstests/fixtures/posters-catalog.jsontests/helpers.js
✅ Files skipped from review due to trivial changes (2)
- tests/fixtures/posters-catalog.json
- src/commands/schema.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/events-integration.test.js
…rtable temp path - Upload POST now has 30s timeout (configurable via config.uploadTimeoutMs) - Upload response validated: requires uploadData.url string - Download checks content-length + buffer size against 10MB limit - Temp path uses os.tmpdir() instead of hardcoded /tmp/ Addresses CodeRabbit review threads.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5
What
New
posterscommand group + image support onevents create/update.New Commands
partiful posters list— Browse poster library (filter by--category,--type,--limit)partiful posters search <query>— Fuzzy search by tags/name/categoriespartiful posters get <id>— Full poster detailsNew Flags on
events createandevents update--poster <id>— Set a built-in poster by ID--poster-search <query>— Search poster library, use best match--image <path|url>— Upload a custom image from file path or URLHow It Works
--poster/--poster-search--image ./photo.png--image https://...Examples
Testing
Hardening (from CodeRabbit review)
os.tmpdir()for cross-platform compatibility--limitvalidated as positive integerResearch
docs/research/2026-03-24-event-image-schema.md— full API research from browser interceptionSummary by CodeRabbit
New Features
Documentation
Tests