refactor: extract shared event helpers, eliminate duplication#51
Merged
refactor: extract shared event helpers, eliminate duplication#51
Conversation
Changes: - Extract shared lib/events.js with: - buildBaseEvent(): event payload construction (was duplicated in events.js, bulk.js, clone.js) - confirm(): readline prompt (was duplicated in events.js and blasts.js) - toFirestoreMap(): Firestore field conversion (moved from events.js) - resolvePosterImage(): poster lookup logic (was duplicated in create/update/clone) - resolveUploadImage(): image upload handling (was duplicated in create/update/clone) - validateImageOptions(): mutual exclusivity check - buildLinks(): link array construction - DEFAULT_GUEST_STATUS_COUNTS: was copy-pasted 3x - events.js: 824 → 558 lines (-32%), extracted makePayload() and handleError() helpers - bulk.js: 315 → 275 lines, now uses shared buildBaseEvent instead of local duplicate - blasts.js: 118 → 106 lines, removed duplicate confirm() and unused MAX_BLASTS_PER_EVENT - Fixed: bulk.js catch block was dynamically importing PartifulError (inconsistent) - Fixed: errors from poster/image helpers now throw typed errors (NotFoundError, ValidationError) instead of generic Error, preserving correct exit codes Total: 3885 → 3729 lines (-156, -4%) Files modified: 4 (3 changed + 1 new)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtracted event/image/upload/confirmation and Firestore-mapping helpers into a new shared module ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/bulk.js (1)
105-120:⚠️ Potential issue | 🟠 MajorBulk poster columns are silently ignored now.
normalizeRow()still acceptsposterandposterSearch, but both the dry-run path and the live create loop now only usebuildBaseEvent(n).event. SincebuildBaseEvent()never resolves media, rows with those columns will succeed and create events without the requested poster. Either resolve them here with the shared poster helpers or reject those columns explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/bulk.js` around lines 105 - 120, The dry-run and live create paths are currently using only buildBaseEvent(n).event which ignores poster/posterSearch resolved by normalizeRow; update both paths to resolve poster media before sending: in the dry-run branch call the shared poster-resolution helper (the same logic used elsewhere to turn poster/posterSearch into resolved media) and include that resolved poster in the object returned from buildBaseEvent for jsonOutput, and in the live-create loop replace payload = { data: wrapPayload(config, { event }) } with a payload that includes the resolved poster (e.g., resolvePoster/attachPoster result merged into event) so poster/posterSearch are honored; alternatively, if poster/posterSearch are unsupported here, explicitly validate normalizeRow output and throw/reject when poster or posterSearch are present so callers get a clear error instead of silent omission.
🤖 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/commands/events.js`:
- Around line 18-29: The bulk create code currently builds the payload as {
data: wrapPayload(config, { event }) } which sends payload.data.event instead of
payload.data.params.event; import makePayload from events.js and replace that
inline payload with makePayload(config, { event }) so the wrapper produces
payload.data.params.event; update the bulk create call site that constructs the
payload (the code that uses wrapPayload for events) to call makePayload(config,
{ event }) and remove the direct wrapPayload usage for this case.
In `@src/lib/events.js`:
- Around line 59-66: buildBaseEvent serializes startDate/endDate with the
machine timezone because parseDateTime ignores the timezone parameter; update
the implementation so the timezone parameter is honored before producing the ISO
string. Specifically, modify parseDateTime (in src/lib/dates.js) to
construct/convert the Date in the provided timezone (or return a timezone-aware
ISO) and then have buildBaseEvent continue to call parseDateTime(opts.date,
opts.timezone) and use the returned value.toISOString(); ensure parseDateTime
uses the timezone argument to calculate the correct instant so event.startDate /
event.endDate match event.timezone.
- Around line 42-47: The exported function validateImageExtension is broken:
replace the incorrect await_extname_sync call and wrong variable usage by
importing/using Node's path.extname and computing const ext =
path.extname(filePath).toLowerCase(); then check if
ALLOWED_IMAGE_EXTENSIONS.includes(ext) and throw the Error with the existing
message if not; alternatively, if the function is unused across the codebase you
can remove the exported validateImageExtension symbol instead. Ensure the
implementation references the function name validateImageExtension and the
variable ext so callers get a working check.
---
Outside diff comments:
In `@src/commands/bulk.js`:
- Around line 105-120: The dry-run and live create paths are currently using
only buildBaseEvent(n).event which ignores poster/posterSearch resolved by
normalizeRow; update both paths to resolve poster media before sending: in the
dry-run branch call the shared poster-resolution helper (the same logic used
elsewhere to turn poster/posterSearch into resolved media) and include that
resolved poster in the object returned from buildBaseEvent for jsonOutput, and
in the live-create loop replace payload = { data: wrapPayload(config, { event })
} with a payload that includes the resolved poster (e.g.,
resolvePoster/attachPoster result merged into event) so poster/posterSearch are
honored; alternatively, if poster/posterSearch are unsupported here, explicitly
validate normalizeRow output and throw/reject when poster or posterSearch are
present so callers get a clear error instead of silent omission.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2f7a3e6-8d7e-45ad-8e3e-1e2b4cd48325
📒 Files selected for processing (4)
src/commands/blasts.jssrc/commands/bulk.jssrc/commands/events.jssrc/lib/events.js
Comment on lines
+59
to
+66
| export function buildBaseEvent(opts) { | ||
| const startDate = parseDateTime(opts.date, opts.timezone); | ||
| const endDate = opts.endDate ? parseDateTime(opts.endDate, opts.timezone) : null; | ||
|
|
||
| const event = { | ||
| title: opts.title, | ||
| startDate: startDate.toISOString(), | ||
| timezone: opts.timezone || 'America/Los_Angeles', |
There was a problem hiding this comment.
buildBaseEvent() still serializes dates in the process timezone.
buildBaseEvent() passes opts.timezone into parseDateTime(), but src/lib/dates.js Lines 6-60 never use that parameter when constructing the Date. So these lines can emit startDate/endDate in the machine timezone while also setting event.timezone to the requested zone. Now that create, clone, and bulk all share this helper, --timezone can shift events by hours.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/events.js` around lines 59 - 66, buildBaseEvent serializes
startDate/endDate with the machine timezone because parseDateTime ignores the
timezone parameter; update the implementation so the timezone parameter is
honored before producing the ISO string. Specifically, modify parseDateTime (in
src/lib/dates.js) to construct/convert the Date in the provided timezone (or
return a timezone-aware ISO) and then have buildBaseEvent continue to call
parseDateTime(opts.date, opts.timezone) and use the returned
value.toISOString(); ensure parseDateTime uses the timezone argument to
calculate the correct instant so event.startDate / event.endDate match
event.timezone.
…mezone docs - Fix bulk create payload: use makePayload() wrapper so /createEvent gets data.params.event (not data.event) - Remove broken validateImageExtension() — called nonexistent function, was never used anywhere - Document timezone limitation in dates.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/commands/bulk.js`:
- Around line 150-151: Create a small helper named handleError(thrown) that
normalizes any thrown value before calling jsonError: if thrown is a
PartifulError call jsonError(thrown.message, thrown.exitCode, thrown.type,
thrown.details); else if thrown is an Error call jsonError(thrown.message ||
String(thrown), /*exitCode*/1); else if typeof thrown === 'string' call
jsonError(thrown, 1); otherwise call jsonError(JSON.stringify(thrown, null, 2)
|| String(thrown), 1). Replace the two existing branches in src/commands/bulk.js
(the spots currently doing `if (e instanceof PartifulError) ... else
jsonError(e.message)`) to just call handleError(e) so non-Error throw values are
safely handled.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: abd0ebe3-16a4-4401-8b9c-0064e003756b
📒 Files selected for processing (3)
src/commands/bulk.jssrc/lib/dates.jssrc/lib/events.js
✅ Files skipped from review due to trivial changes (2)
- src/lib/dates.js
- src/lib/events.js
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.
Summary
Full code review and cleanup pass on the partiful-cli codebase. Main focus: eliminating duplication across the largest files.
Changes Made
New file:
src/lib/events.js(shared helpers)Extracted repeated patterns into a single shared module:
buildBaseEvent()— event payload construction (was duplicated in events.js, bulk.js, and helpers/clone.js)confirm()— readline yes/no prompt (was duplicated in events.js and blasts.js)toFirestoreMap()— recursive Firestore field conversion (moved from events.js to shared lib)resolvePosterImage()— poster catalog lookup (was copy-pasted across create/update/clone)resolveUploadImage()— image file/URL upload handling (was copy-pasted across create/update/clone)validateImageOptions()— mutual exclusivity check for --poster/--poster-search/--imagebuildLinks()— link array construction from CLI optionsDEFAULT_GUEST_STATUS_COUNTS— was literally copy-pasted as a 16-key object 3 timessrc/commands/events.js(824 → 558 lines, -32%)makePayload()andhandleError()to reduce repeated boilerplatereadlineandpathimportssrc/commands/bulk.js(315 → 275 lines)buildBaseEvent()instead of localbuildEventPayload()duplicatesrc/commands/blasts.js(118 → 106 lines)confirm()function, now imports from shared libMAX_BLASTS_PER_EVENTconstant (was defined but never used)Bug fixes
NotFoundError,ValidationError) instead of genericError, preserving correct exit codes in JSON outputIssues Found But Not Fixed
helpers/clone.jsduplicates ~80% ofevents clone— these are two separate commands (+clonevsevents clone) with slightly different option sets. Merging them would change the public CLI interface.generateSeries()in bulk.js is defined but unused (dead code for a feature that lives in events create --repeat). Kept it since it may be intended for future bulk series support.jsonOutput()— intentional since it also writes a human-readable table to stderr.Metrics
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation